AsyncMiddleManServlet response flushing

[iiliev2]

Jetty 12
ee8

Java 21

Continuing the discussion from #12294 (comment) as this is a separate topic.

The proxy servlets(in particular AsyncMiddleManServlet, as that is what I am looking at) do not seem to flush responses as the proxy client receives new bytes. Ideally it should auto flush in an optimal way(for ex. if http1.1 and chunked, it should flush on each full chunk). Otherwise there could be huge delays between when the data is available and when it is actually returned to the caller(due to buffering). Since this is a proxy, I expected this to be the default, otherwise I hope to be able to set it up that way.

The ContentTransformer does not seem to provide a way to control this(suggested in the previous github question). That abstraction is about mutating the raw data in some way.

The only way that I can see from the code of AsyncMiddleManServlet is to call flush on the ServletOutputStream right after ProxyWriter calls writeProxyResponseContent. Unfortunatly that method is package private so the only way is to override the entire onWritePossible and flush afterward.

What would you advise is the right way to do this?

[sbordet]

AsyncMiddleManServlet is a proxy servlet dedicated to mutate content.

If you don't need to mutate content, don't use it, and use instead ProxyHandler (recommended) or AsyncProxyServlet.

As for AsyncMiddleManServlet, did you actually follow the advice I gave in the comment?
Does it not work?
If it does not work, we need details, because it is supposed to work like you expect.

[iiliev2]

As for AsyncMiddleManServlet, did you actually follow the advice I gave in the comment?

AsyncMiddleManServlet already uses a default(no-op) ContentTransformer. I do not understand how overriding that will help me for this issue.

Does it not work?

I have created a unit test which illustrates the issue. It does not work.
Uncomment this to fix the test. Is this a bug?

[sbordet]

It's not a bug, perhaps a missing feature.

AsyncMiddleManServlet performs a ServletOutputStream.write() which may be buffered, and there is no explicit API in AsyncMiddleManServlet to allow applications to force the flush of the bytes.

Your current workaround would be to call HttpServletResponse.setBufferSize(<small-value>), so that your writes will not be buffered.

There actually exist AsyncMiddleManServlet.writeProxyResponseContent() but it is package-private.
If it was at least procteted, you could override it, call super and then call output.flush().

So your final solution would be:

class MyAMMS extends AsyncMiddleManServlet {
  protected void writeProxyResponseContent(ServletOutputStream output, ByteBuffer content) throws IOException {
    super.writeProxyResponseContent(output, content);
    if (output.isReady()) {
      output.flush();
    }
  }
}

@iiliev2 do you want to make this contribution?
That is, to create a PR making writeProxyResponseContent() to be protected.

[iiliev2]

output.flush();

I found this old thread where you are discussing how to do async flush:

You have to treat it exactly as a write, by calling isReady() before calling flush()

I can't find any information explaining why that is. Could you please clarify this? I don't see this being part of the servlet spec. I see in the implementation of HttpOutput.isReady that it mutates some state(so it's more like becomeReady).

Do I have to call isReady()? What happens if it returns false? Will it wait for the next chunk to arrive, or it will be notified by the framework when it becomes ready(I think that is what should happen). Currently my workaround(which you said I should not do) occasionally hits an error(without calling isReady). Will this not happen if we could override writeProxyResponseContent?:

java.lang.IllegalStateException: isReady() not called: s=OPEN,api=ASYNC,sc=false,e=null
    at org.eclipse.jetty.server.HttpOutput.flush(HttpOutput.java:712)
    at com.iviliev.Servlet$1.onWritePossible(Servlet.java:78)
    at org.eclipse.jetty.server.HttpOutput.run(HttpOutput.java:1494)
    at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:1469)
    at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:597)
    at org.eclipse.jetty.server.HttpChannel.run(HttpChannel.java:461)
    at 

Apart from that, I have added an additional test in my repo testChunkedTransferAbort to verify that if the client aborts while the streaming is going on, the proxy will kill its proxy client request in a reasonable and predictable amount of time.

All tests are now also executed both via http1.1 and http2.

For http1.1 the flushing is also needed for the new test to succeed. Otherwise the proxying continues even though the client may be long gone.
For http2 the new test works even without the flushing. Do you know why is that? Is it part of the protocol when one side aborts?

[sbordet]

In the email thread you linked, @gregw suggested that flushes are asynchronous, so yes you have to call isReady().

This is because if you did an asynchronous write, and the write did not complete, then isReady() would return false (and schedule for onWritePossible() to be called when the write completes).

If you perform the sequence:

write(large); // does not complete, isReady==false
flush(); // Throws IllegalStateException

I have corrected the above comment with the right code, and yes you are occasionally hitting exactly this problem -- sorry I suggested you the incorrect solution.

If the call to isReady() before flush() returns false it means that the write has been initiated but has not yet finished.
For your case, this is fine because the write to the network is actually in progress.

When an HTTP/1 client aborts, the server is typically not informed. Only by forcing a write the server can detect that the connection is broken.

On the contrary, with HTTP/2 the server is informed that the request has been canceled, so it may cancel the request even when no writes are in progress.

Just to reiterate: I suggest you do not override ProxyWriter.onWritePossible().
Instead, either reduce the response buffer size, or contribute a PR to make writeProxyResponseContent() protected (and override that), or wait for us to do that.

[iiliev2]

with HTTP/2 the server is informed that the request has been canceled, so it may cancel the request

Are you referring to graceful termination, initiated by the peer(in my test - the client)?

I would like to check what would happen if there is an issue with the peer and the RST_STREAM does not reach the proxy. Is there a way to test this with the jetty client? For ex. if there were a bug in the client, aborting requests without properly notifying the proxy and continuing to issue further requests later(ie. not closing the TCP connection).

[sbordet]

I am referring to RST_STREAM.

The term "graceful termination" is used when sending GOAWAY, so I would not confuse the two.

If the client stops sending frames for a particular stream, eventually the server-side (the proxy in your case) would timeout the stream, and issue a RST_STREAM to the client.
The connection is not affected, it remains open.
We have a bunch of tests for this behavior.

[sbordet]

@iiliev2 let us know if you would like to contribute the improvement discussed above, otherwise we'll do it.

[iiliev2]

I can contribute a fix toward the 10.x branch. Is that possible?

[sbordet]

@iiliev2 no, we only accept contributions to the 12.0.x branch.

[iiliev2]

@sbordet I just noticed something suspicious. AsyncMiddleManServlet.ProxyWriter#chunks = new ArrayDeque<>(), which is not a threadsafe class, looks like is getting modified concurrently:
this.chunk = chunk = chunks.poll();

proxyWriter.offer(buffer, counter);

Am I right, what am I missing?

[iiliev2]

It's not a bug, perhaps a missing feature.

AsyncMiddleManServlet performs a ServletOutputStream.write() which may be buffered, and there is no explicit API in AsyncMiddleManServlet to allow applications to force the flush of the bytes.

Your current workaround would be to call HttpServletResponse.setBufferSize(<small-value>), so that your writes will not be buffered.

There actually exist AsyncMiddleManServlet.writeProxyResponseContent() but it is package-private. If it was at least procteted, you could override it, call super and then call output.flush().

So your final solution would be:

class MyAMMS extends AsyncMiddleManServlet {
  protected void writeProxyResponseContent(ServletOutputStream output, ByteBuffer content) throws IOException {
    super.writeProxyResponseContent(output, content);
    if (output.isReady()) {
      output.flush();
    }
  }
}

@iiliev2 do you want to make this contribution? That is, to create a PR making writeProxyResponseContent() to be protected.

Related to this, I still have some doubts that this will be sufficient. If isReady returns false before the flush, the servlet container will call AsyncMiddleManServlet.ProxyWriter#onWritePossible later, at which point it should continue from where it left off, ie flush first, then do the whole poll chunk-write-flush procedure again. Is this right?

[sbordet]

I just noticed something suspicious. AsyncMiddleManServlet.ProxyWriter#chunks = new ArrayDeque<>(), which is not a threadsafe class, looks like is getting modified concurrently.

Good catch.

Related to this, I still have some doubts that this will be sufficient. If isReady returns false before the flush, the servlet container will call AsyncMiddleManServlet.ProxyWriter#onWritePossible later, at which point it should continue from where it left off, ie flush first, then do the whole poll chunk-write-flush procedure again. Is this right?

Let's assume a previous write actually hit the network, and got TCP congested, so isReady() returns false, so you won't flush() (no need to, it is already writing to the network).

Later, onWritePossible() would be called, which means that the previous write now completed, and we can poll another chunk, let's assume it's a small one.
Then writeProxyResponseContent() would be called, you call super to perform the write, which will be buffered, isReady()==true so you would flush().

Now, either the flush() completes the write, so isReady()==true, and the loop continues.
Or, flush() goes again in TCP congestion, isReady()==false, and the loop will stop, but schedule onWritePossible() to be called again when the write completes.

Everything should work fine.

I'll make the changes for jetty-12.0.x, would be great if you can test them:

• fix the queue locking
• make writeProxyResponseContent() protected.

[sbordet]

@iiliev2 would you be able to try #12542 and report back if it works for you?