Fixes #12323 - AsyncMiddleManServlet response flushing. #12542
Conversation
* Fixed `ProxyWriter.chunks` locking. * Made `writeProxyResponseContent()` protected so it can be overridden to flush the response content. * Added test case. Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
iiliev2
left a comment
iiliev2
left a comment
There was a problem hiding this comment.
What about the fields
private Chunk chunk;
private boolean writePending;shouldn't those be volatile?
| if (_log.isDebugEnabled()) | ||
| _log.debug("{} proxying content to downstream: {} bytes {}", getRequestId(clientRequest), content.remaining(), callback); | ||
| return chunks.offer(new Chunk(content, callback)); | ||
| try (AutoLock ignored = lock.lock()) |
There was a problem hiding this comment.
What is the benefit of this solution, rather than just using something like ConcurrentLinkedDeque for the chunks ?
| try (AutoLock ignored = lock.lock()) | ||
| { | ||
| this.chunk = chunk = chunks.poll(); | ||
| } |
There was a problem hiding this comment.
Shouldn't the lock be released after writeProxyResponseContent? I am not sure, but it seems that two threads may poll a chunk(ie there were two chunks in the queue) and then they may race and write the bytes out of order.
| protected class ProxyWriter implements WriteListener | ||
| { | ||
| private final AutoLock lock = new AutoLock(); | ||
| private final Queue<Chunk> chunks = new ArrayDeque<>(); |
There was a problem hiding this comment.
ProxyResponseListener.buffers also looks like a collection which needs to be threadsafe.
… races. Signed-off-by: Simone Bordet <[email protected]>
|
@iiliev2 we have reworked
|
|
There is nothing in private boolean hasContent;
private long contentLength;
private long length;
private Response response; |
No, it does not. The serialization mechanism ensures enough to guarantee visibility. Regarding the fact that listeners are invoked serially, yes we can improve the documentation, but it is quite natural that events flow sequentially, as they are read from the network. |
lorban
left a comment
lorban
left a comment
There was a problem hiding this comment.
There's a few minor improvements that could improve readability/maintainability, otherwise LGTM.
...-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java
Show resolved
Hide resolved
...-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private static class Chunk | ||
| private record Chunk(ByteBuffer buffer, Callback callback) |
There was a problem hiding this comment.
Chunk is quite a loaded term now, maybe this record should be renamed to something like BufferWithCallback or something to avoid confusion with Content.Chunk?
...-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AsyncMiddleManServlet.java
Show resolved
Hide resolved
Signed-off-by: Simone Bordet <[email protected]>
lorban
left a comment
lorban
left a comment
There was a problem hiding this comment.
I'd appreciate the extra nullings and assertions, but this LGTM the way it is.
ProxyWriter.chunkslocking.writeProxyResponseContent()protected so it can be overridden to flush the response content.