Issue #14403 - Serialize FCGI input buffer access - #15616
Conversation
lorban
left a comment
lorban
left a comment
There was a problem hiding this comment.
You added a very coarse lock and because of its broad scope, this happens to fix the test, or make it much less likely to reproduce (I haven't checked that).
You're freely calling non-local methods and you did not modify all the methods accessing the member variables under the lock's scope.
I'm sorry but this isn't a proper fix, but it looks more like a random attempt at throwing a lock at the problem and hoping that this will make the problem go away.
If you believe you figured out how threads interact with each other to make the reported problem happen, you should then figure out a proper locking strategy with minimal lock scopes.
| // even if the buffer has been fully consumed because releaseInputBuffer() | ||
| // must be called as the last release for it to be able to null out the | ||
| // inputBuffer field exactly when the latter isn't used anymore. | ||
| if (parse(inputBuffer.getByteBuffer())) |
There was a problem hiding this comment.
You cannot call the parser with a lock held, this would lead to deadlocks.
| else if (read == 0) | ||
| { | ||
| releaseInputBuffer(); | ||
| fillInterested(fillableCallback); |
There was a problem hiding this comment.
Calling any method of another class (even a super one) with a lock held is an anti-pattern. All around Jetty's codebase we made sure to never do that.
| else | ||
| { | ||
| releaseInputBuffer(); | ||
| shutdown(); |
There was a problem hiding this comment.
Ditto: you cannot call another class' method with a lock held.
| // even if the buffer has been fully consumed because releaseInputBuffer() | ||
| // must be called as the last release for it to be able to null out the | ||
| // inputBuffer field exactly when the latter isn't used anymore. | ||
| if (parse(inputBuffer.getByteBuffer())) |
There was a problem hiding this comment.
Ditto: you cannot call the parser with a lock held.
| { | ||
| releaseInputBuffer(); | ||
| if (failure == null) | ||
| fillInterested(fillableCallback); |
There was a problem hiding this comment.
Ditto: you cannot call another class' method with a lock held.
| if (failure == null) | ||
| fillInterested(fillableCallback); | ||
| else | ||
| getFlusher().shutdown(); |
There was a problem hiding this comment.
Ditto: you cannot call another class' method with a lock held.
|
@lorban Thanks for the review. I reworked the fix in #15648 to defer application dispatch until I am closing this PR in favor of the new one. Please feel free to let me know if you spot any issues. |
2 participants
Fixes #14403.
ServerFCGIConnectionowns a single parser and input buffer, while request completion may run on a different server thread whileonFillable()orparseAndFill()is still using that buffer. This can release the connection-owned buffer before input processing has completed.Serialize those connection-level input-processing and completion paths with a per-connection
AutoLock. Add a deterministic regression test that holds an in-progress fill and verifies completion waits before releasing the buffer.Tests:
ServerFCGIConnectionTestServerFCGIConnectionTest,HttpClientTest(24/24)RoundRobinConnectionPoolTest#testMultiplexWithMaxUsage(6/6 transports)