Issue #14403 - Defer FCGI application dispatch until the parser returns - #15648
Issue #14403 - Defer FCGI application dispatch until the parser returns#15648DragonFSKY wants to merge 3 commits into
Conversation
…returns Signed-off-by: Dongliang Xie <dragonfsky@gmail.com>
lorban
left a comment
lorban
left a comment
There was a problem hiding this comment.
It looks like taking care of the TODO: here we just execute the task. in onHeaders is the actual meat of this fix, and the right thing to do: like for other protocols, wait until the parser has returned before dispatching.
The tests are head-scratching, though. They're hard to read (that's fair, race conditions are complicated to test for) seem to do a lot of unusual stuff (the finally block in testApplicationDispatchedAfterParsing() should not be needed).
Did you use AI to analyze this problem and/or generate the fix/tests? If so, you should at the very least explicitly state that. Because this PR is borderline AI slop.
| { | ||
| getExecutor().execute(task); | ||
| } | ||
| catch (RejectedExecutionException x) |
There was a problem hiding this comment.
This should never happen as the Jetty connection pool assumes the queue task is infinite. Plus, there's already a catch-all block that is supposed to do the right thing.
| if (stream == null && inputBuffer.isEmpty()) | ||
| releaseInputBuffer(); | ||
| return; | ||
| break; |
There was a problem hiding this comment.
Why breaking instead of doing the onRequest check and dispatch here? This makes the method slightly less readable.
| CountDownLatch inputReleaseProceed = new CountDownLatch(1); | ||
|
|
||
| ArrayByteBufferPool.Tracking trackingPool = new ArrayByteBufferPool.Tracking(); | ||
| ByteBufferPool bufferPool = new ByteBufferPool.Wrapper(trackingPool) |
There was a problem hiding this comment.
Why re-wrapping the tracking pool to do a similar job to what it already does?
| } | ||
| finally | ||
| { | ||
| inputReleaseProceed.countDown(); |
There was a problem hiding this comment.
What are you trying to do here? Does that really need to be in a finally block?
| finally | ||
| { | ||
| inputReleaseProceed.countDown(); | ||
| fillThread.join(TimeUnit.SECONDS.toMillis(10)); |
There was a problem hiding this comment.
What's the rationale behind that tiny timeout?
| } | ||
|
|
||
| @Test | ||
| public void testApplicationDispatchRejectedBeforeRequestEnd() throws Exception |
There was a problem hiding this comment.
There is no need to test for that as this should never happen in practice.
| } | ||
| } | ||
|
|
||
| private static void awaitLatch(CountDownLatch latch, String message) |
There was a problem hiding this comment.
There is no need for this static helper, the boolean returned by latch.await() should just be tested with assertTrue().
| await().atMost(10, TimeUnit.SECONDS).untilAsserted(() -> | ||
| assertThat("Server Leaks: " + trackingPool.dumpLeaks(), trackingPool.getLeaks().size(), is(0))); | ||
| } | ||
|
|
There was a problem hiding this comment.
All the following helpers add more confusion than value and should be inlined IMHO. It's test code after all, self-contained tests usually are more readable. There is also little value in extracting out functions that are (almost) not reused.
Signed-off-by: Dongliang Xie <dragonfsky@gmail.com>
|
Yes, I did use an AI coding assistant here, and I didn’t review its output carefully enough before opening the PR. That was my mistake, and I’m sorry for creating unnecessary review work for you. I’ve now removed the unnecessary rejection handling and test scaffolding. I’ll make sure future changes are carefully reviewed, simplified, and consistent with the existing code and test style before I submit them. |
3 participants
Fixes #14403.
HttpStreamOverFCGI.onHeaders()currently submits the application task from inside aServerParser.parse()callback. A fast or inline task can therefore complete the request and release the connection input buffer beforeonFillable()has finished the current parser bookkeeping.Return the task to
ServerFCGIConnectionand dispatch it only afterServerParser.parse()has returned and the input-buffer bookkeeping is complete. Clear the one-shot handoff before dispatch so inline execution cannot observe a stale task.The regression test uses an inline executor to reproduce the old reentrant completion without additional thread or buffer-pool instrumentation. It fails on the target branch with the input buffer already released and passes with no tracked buffer leaks after this change.
Tests:
ServerFCGIConnectionTest(1/1)jetty-fcgi-servertests excludingExternalFastCGIServerTest(24/24)RoundRobinConnectionPoolTest#testMultiplexWithMaxUsageacross all 6 transportsAI disclosure
I used OpenAI GPT-5.6 and Kimi K3 to assist with issue analysis and drafting the implementation and tests. I reviewed and verified the final changes.