Add missing integration between ServletContextResponse.write() and HttpOutput

[lorban]

ServletContextResponse and HttpOutput should be kept in sync w.r.t how many bytes were written (to apply the content-length logic) and their closed state / last flag handling.

Problem Description

The problem is exposed by the newly added ResourceServletCompressionCompleteTest: when a Core handler (CompressionHandler with zstd in this case, but any other handler modifying the output would trigger the bug) wraps a EE10/11 ServletContextHandler, any sufficiently large data (larger than the output buffer size) sent by the servlet (ResourceServlet serving a 64 KB file in this case) result in the output being corrupted and the ServletChannel aborting with java.io.IOException: Insufficient content written 0 < 65536.

This happens because the data is sent via the Core Response.write() API, bypassing the HttpOutput entirely such as it did not see a single byte being written, so when the ServletChannel calls HttpOutput.isContentIncomplete() that method returns true and the servlet channel aborts the request, thinking no byte was written.

This change fixes the problem by ensuring 3 things:

• Making sure that any data written directly via ServletContextResponse.write() is accounted for in HttpOutput.
• Fixing the missing callback completions in InputStreamWritingCB and ReadableByteChannelWritingCB so that the straight-through HttpOutput.sendContent() calls can't deadlock.
• Closing the HttpOutput when ServletContextResponse.write() sees the last write being written.

Fixes #15678

[joakime]

Is this a better (generic) replacement for PR #15433 ?

Seems like this will apply to all compression libs equally.

[lorban]

@joakime although this PR fixes a bug that impacts all compressors, I don't think #15433 is a duplicate; it looks more like a different bug only present in brotli.

[gregw]

Early feedback that I don't like the name at least. I need to understand more before I can say if the approach is OK.

[gregw]

This is badly named and it is nasty that it needs to be protected.
If I'm reading the code correctly it is called by the Servlet streams before the last write is done. So it is kind of a notification that the last write will be done? But then it is calling the callback before the write is done?

So it needs a better name at least... but a solution that doesn't need this call at all would be better. Can you add some more detail in the PR about what the actual issue is and how this solves it?

[lorban]

This is called by the ServletContextResponse when it is called by the Core API the last write is being done but the HttpOutput has not yet been closed.

I've updated the PR with a better description of the problem, as accurately as my mind reminds it. Hopefully that should help you better understand the context.

[gregw]

You should be able to pass all the state you need here simply with:

Suggested change

	if (!httpOutput.isClosed() && last)
	httpOutput.lastWriteComplete();
	httpOutput.addBytesWritten(BufferUtil.length(content));
	httpOutput.addBytesWritten(BufferUtil.length(content), last);

[gregw]

Actually, you really should not be messing with this written mechanism. It is there for some very specific TCK test that check that we automagically close when the last byte is written. This is above the level of compression, so it matches any application concept of content-length with the bytes written.

Compression will change both.

So we need to better understand the problem as this looks like the wrong solution.

[lorban]

I did not think of looking at the TCK tests, but yes, I should as this mechanism is quite sensible. Any hints about which ones to look at from the top of your mind?

About this new accounting, it is at the root of this bug: if zero byte is written via the HttpOutput but some were written directly via the servlet response, the servlet channel will believe that the written byte count does not match the Content-Length header's value that was previously set by ResourceService.sendData(), intercepted by ServletContextResponse.HttpFieldsWrapper.onAddField() and fed to HttpOutput.setApplicationContentLength() so ServletChannel.handle()'s getHttpOutput().isContentIncomplete() check returens false and the response gets failed.

[gregw]

This looks to be double counting. The aggregated bytes includes those that are written. I.e. how does a byte get aggregated without being written?

[lorban]

This method returns the # of bytes that have been written, either via HttpOutput (that may have buffered some if the written data) or via ServletContextResponse.write(), which is slightly different from the # of bytes already flushed, hence the addition of the yet-to-be-written bytes buffered by the HttpOutput's aggregation.

[gregw]

Actually, you really should not be messing with this written mechanism. It is there for some very specific TCK test that check that we automagically close when the last byte is written. This is above the level of compression, so it matches any application concept of content-length with the bytes written.

Compression will change both.

So we need to better understand the problem as this looks like the wrong solution.

[gregw]

@lorban I can get the test harness to pass with the simple addition of:

    @Override
    public void write(boolean last, ByteBuffer content, Callback callback)
    {
        getHttpOutput().addBytesWritten(content.remaining());
        super.write(last, content, callback);
    }

Which is using addBytesWritten exactly how it should be, plus I need to change the encoder with:

    protected WriteRecord encode(boolean last, ByteBuffer content)
    {
        State initialState = state.get();
        if (initialState == State.FINISHED)
        {
            if (content.hasRemaining())
                throw new IllegalStateException("Already released " + BufferUtil.toDetailString(content));
            return null;
        }
        ...

That second part might be a hack, as it would be better to stop the iteration prior to calling encode again with an empty content. I'm not sure why it is doing that, so perhaps a better fix is to stop that.

Eitherway, I think a much simpler fix is possible.

[lorban]

@gregw with your suggested fix you get 8 failures in HttpOutputTest:

• 7 IllegalStateException: too much content written
• 1 IOException: written 6304 < 10400 content-length

[gregw]

@gregw with your suggested fix you get 8 failures in HttpOutputTest:

• 7 IllegalStateException: too much content written
• 1 IOException: written 6304 < 10400 content-length

Still, I think it is the correct approach. The addBytesWritten API is there precisely to update the count when an alternate path is taken.

I think your approach is messing with that too much and it is very unclear exactly what it is trying to do (See my first comments)

I'll try to find some time this week to look some more.

[gregw]

@lorban I'm now thinking the fix really needs to be in ResourceServlet. It is the one that decides to bypass the ServletResponse and call the core response directly, so it should be responsible for making sure the accounting is done correctly. I'm going to play there for a bit.

However, there is a problem with either compression or the test (@joakime you might want to comment on this). If I fiddle with ResourceServlet so that it always uses the ServletResponse, then I get the exception:

java.lang.IllegalStateException: Already released
	at org.eclipse.jetty.compression.zstandard.internal.ZstandardEncoderSink.encode(ZstandardEncoderSink.java:80)
	at org.eclipse.jetty.compression.EncoderSink$EncodeBufferCallback.process(EncoderSink.java:97)
	at org.eclipse.jetty.util.IteratingCallback.processing(IteratingCallback.java:374)
	at org.eclipse.jetty.util.IteratingCallback.iterate(IteratingCallback.java:351)
	at org.eclipse.jetty.compression.EncoderSink.write(EncoderSink.java:39)
	at org.eclipse.jetty.compression.server.internal.CompressionResponse.write(CompressionResponse.java:192)
	at org.eclipse.jetty.server.Response$Wrapper.write(Response.java:841)
	at org.eclipse.jetty.server.handler.ContextResponse.write(ContextResponse.java:56)
	at org.eclipse.jetty.ee10.servlet.ServletContextResponse.write(ServletContextResponse.java:224)
	at org.eclipse.jetty.ee10.servlet.HttpOutput.channelWrite(HttpOutput.java:238)
	at org.eclipse.jetty.ee10.servlet.HttpOutput.complete(HttpOutput.java:477)
	at org.eclipse.jetty.ee10.servlet.ServletContextResponse.completeOutput(ServletContextResponse.java:212)

I can avoid this by making encode just return null if the passed content is empty and we are last. But we really should not need to do that?? Or should we???

[gregw]

@lorban I made the change to ResourceServlet to make it do its own accounting and it does fix the problem.
See https://github.com/jetty/jetty.project/compare/fix/12.1.x/15678-ResourceServlet-bypass?expand=1

I think the approach needs refinements, but I think it is correct.

[lorban]

@gregw I also think the addBytesWritten() API is the way to go, as a good chunk of these changes are about that.

There are basically 3 changes in this PR:

1. make ServletContextResponse.write() call HttpOutput.addBytesWritten() and modify HttpOutput so the bytes written counter is kept coherent for the content-length checks. This is what the lockedGetWritten() and prepareSendContent() changes are about.
2. fix InputStreamWritingCB.onCompleteSuccess() and ReadableByteChannelWritingCB.onCompleteSuccess() to add a missing super.onCompleteSuccess() call, which solves the Blocking.Callback incomplete warnings and flaky deadlocks when the blocking HttpOutput.sendContent() calls are used
3. advance the HttpOutput state machine when ServletContextResponse.write()'s last flag is true, this is what HttpOutput.lastWriteComplete() and the if (_eof) check in InputStreamWritingCB and ReadableByteChannelWritingCB is about

(1) I think we're in agreement: ServletContextResponse.write() must call HttpOutput.addBytesWritten() and the code reading/updating the counter has to be fixed to account for that change.
(2) is slightly unrelated, but nevertheless a bug that should be fixed.
(3) I do agree that the changes related to lastWriteComplete() might have some edge cases I did not consider (like maybe breaking tests of the servlet TCK), need some more work and possibly should be dropped entirely.

I'm going to work on removing the changes related to (3) entirely to see if the whole Jetty test suite passes without them, or point out what fails. I'm also going to have a look at the servlet TCK to see how it goes with and without these changes.

[lorban]

@lorban I made the change to ResourceServlet to make it do its own accounting and it does fix the problem. See https://github.com/jetty/jetty.project/compare/fix/12.1.x/15678-ResourceServlet-bypass?expand=1

I think the approach needs refinements, but I think it is correct.

I don't get why you'd want to modify the ResourceServlet while the problem is rather generic: any servlet that makes use of the Core API to write would need to carefully add that extra HttpOutput.addBytesWritten() call, wouldn't it? Why not fixing ServletContextResponse/HttpOutput so that they do the right thing instead?

[gregw]

@lorban I made the change to ResourceServlet to make it do its own accounting and it does fix the problem. See https://github.com/jetty/jetty.project/compare/fix/12.1.x/15678-ResourceServlet-bypass?expand=1
I think the approach needs refinements, but I think it is correct.

I don't get why you'd want to modify the ResourceServlet while the problem is rather generic: any servlet that makes use of the Core API to write would need to carefully add that extra HttpOutput.addBytesWritten() call, wouldn't it? Why not fixing ServletContextResponse/HttpOutput so that they do the right thing instead?

Because there are only two options:

1. the core API does the accounting, in which case it must a) know that it is wrapped by a ServletContextResponse; and b) be able to differentiate between calls that are direct vs calls coming via the ServletContextResponse, which can only really be done if there are different APIs and discipline in how they are used.

2. the code that bypasses the servlet API does the accounting. This is the easiest to get right because you can't accidentally bypass the servlet API - you have to do a lot of work to check if you can and then go directly to the core API, so it is not that much more to say do the accounting.

In fact, I don't think you can "fix ServletContextResponse/HttpOutput` to do the right thing", because when you are on the inside, you don't know what the right thing is to do.

[lorban]

However, there is a problem with either compression or the test (@joakime you might want to comment on this). If I fiddle with ResourceServlet so that it always uses the ServletResponse, then I get the exception:

java.lang.IllegalStateException: Already released
	at org.eclipse.jetty.compression.zstandard.internal.ZstandardEncoderSink.encode(ZstandardEncoderSink.java:80)
	at org.eclipse.jetty.compression.EncoderSink$EncodeBufferCallback.process(EncoderSink.java:97)
	at org.eclipse.jetty.util.IteratingCallback.processing(IteratingCallback.java:374)
	at org.eclipse.jetty.util.IteratingCallback.iterate(IteratingCallback.java:351)
	at org.eclipse.jetty.compression.EncoderSink.write(EncoderSink.java:39)
	at org.eclipse.jetty.compression.server.internal.CompressionResponse.write(CompressionResponse.java:192)
	at org.eclipse.jetty.server.Response$Wrapper.write(Response.java:841)
	at org.eclipse.jetty.server.handler.ContextResponse.write(ContextResponse.java:56)
	at org.eclipse.jetty.ee10.servlet.ServletContextResponse.write(ServletContextResponse.java:224)
	at org.eclipse.jetty.ee10.servlet.HttpOutput.channelWrite(HttpOutput.java:238)
	at org.eclipse.jetty.ee10.servlet.HttpOutput.complete(HttpOutput.java:477)
	at org.eclipse.jetty.ee10.servlet.ServletContextResponse.completeOutput(ServletContextResponse.java:212)

I can avoid this by making encode just return null if the passed content is empty and we are last. But we really should not need to do that?? Or should we???

IllegalStateException: Already released is thrown when EncoderSink.write() is called more than once with the last flag set to true. Working on not throwing this exception is working around a problem that got introduced elsewhere.

Update: calling HttpOutput.close() results in ServletContextResponse.write(true, EMPTY, callback). Since ResourceServlet already did the last write bypassing HttpOutput, this explain why I added this extra HttpOutput.lastWriteComplete() method that is called when the ResourceServlet does the last write to mark the HttpOutput as closed so followup calls to close() en up being NOOPs.

[lorban]

I don't get why you'd want to modify the ResourceServlet while the problem is rather generic: any servlet that makes use of the Core API to write would need to carefully add that extra HttpOutput.addBytesWritten() call, wouldn't it? Why not fixing ServletContextResponse/HttpOutput so that they do the right thing instead?

Because there are only two options:

1. the core API does the accounting, in which case it must a) know that it is wrapped by a `ServletContextResponse`; and b) be able to differentiate between calls that are direct vs calls coming via the `ServletContextResponse`, which can only really be done if there are different APIs and discipline in how they are used.

2. the code that bypasses the servlet API does  the accounting.  This is the easiest to get right because you can't accidentally bypass the servlet API - you have to do a lot of work to check if you can and then go directly to the core API, so it is not that much more to say do the accounting.

In fact, I don't think you can "fix ServletContextResponse/HttpOutput` to do the right thing", because when you are on the inside, you don't know what the right thing is to do.

If we were discussing unwrapping ServletContextResponse, I would agree with you.

But in this case, the ResourceServlet bypasses the servlet API for writing, but it writes to ServletContextResponse, it doesn't unwrap the Core response. I don't see what's unreasonable about having the core-to-servlet glue code having to do a bit of extra work to keep content-length consistent no matter if you use servlet API or directly call ServletContextResponse.write().

[gregw]

@lorban said:

If we were discussing unwrapping ServletContextResponse, I would agree with you.

But ServletContextResponse is a core response, it is just one that is scoped to a ServletContext, so it is below the level of the HttpOutput, which is implementing the ServletOutputStream and specifically the servlet requirements about content length and bytes written.

I guess we could move the accounting to ServletContextResponse, but then it would not work if we wrap anything between the API (HttpOutput) and the ServletContextResponse, as the requirement is that we count bytes at the API, not what is written to the lower layers.

So the ResourceServlet is explicitly bypassing the ServletOutStream API implementation and going to a core Response. OK it is a core response that knows it is in a ServletContext but not for API purposes, only for contextee type stuff like wrapping callbacks with classloaders etc.

However you think of the bypass, it is a bypass. In this case it is bypassing the hard requirement for us to count bytes at the API level, not at the lower levels. I think the bypasser needs to be responsible.

[sbordet]

@lorban @gregw

• ServletCoreResponse.write() duplicates the logic about wrapping or not present in ResourceServlet.doGet().
• That logic is retrieved from ServletContextResponse.
• The ServletChannel state machine invokes the Servlet in DISPATCH state, and then in COMPLETE state invokes ServletContextResponse.completeOutput().
• Given the above, the common place to put the logic to handle bypass writes seems to be ServletContextResponse, where in case of bypass it can: A) forward the count to HttpOutput, and B) skip completeOutput().

[gregw]

@sbordet I'm all for avoiding code duplication and putting the bypass code somewhere common, but....

I do not see how that can be done in ServletContextResponse, as it does not know if a call to write is a bypass one or not. The only way to make it work, is to move all the accounting logic into ServletContextResponse, but then that doesn't work when that response is wrapped by something doing the compression - as the requirement is to count the bytes at the API (HttpOutput) not count the compressed bytes at the inner write.

You can see that the bypass code in ServletCoreResponse correctly did the accounting when doing the bypass. The problem is just that the "duplicate" code in ResourceHandler does not. Simplest fix is to make it do that accounting (my branch). Next simplest fix is to factor out the common bypass code somewhere. Complicated "fix" that will break the TCK (and other things we will not discover for some time) is to completely change the way it has worked for decades and move the accounting away from the API.

I think we should do the simple fix for now, to just fix this issue. Then look at the common code fix later.

Putting the common code on HttpOutput may be the best way forward. Either with a new method for doing a bypass write... or potentially within the existing (but private) private void channelWrite(ByteBuffer content, boolean last, Callback callback) method, which could have the accounting could be moved into that method too? Putting the bypass on HttpOutput means that any knowing code passed an OutputStream could attempt the downcast and bypass (with that being in a static method on HttpOutput).

[lorban]

@gregw I've taken your branch, added the missing fixes for the NestedChannelWriteCB not waking up the blockers (+ modified HttpOutputTest to catch those) and the bypass of HttpOutput completion when the latter has been bypassed to avoid a double last buffer write (this fixes the problem that you worked around in ZstandardEncoderSink) and created a PR from it so we can compare solutions.

Basically, if you agree in substance with that other branch, the only disagreement left is where to put the missing addBytesWritten() call, and I'm then much more in agreement with your "small, safe fix leaving a bit of duplication for now, rework that later" statement.

[lorban]

Superseded by #15847