-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix buffer leak in HttpClientStreamTest.testUploadWithConcurrentServerCloseClosesStream()
#10431
Fix buffer leak in HttpClientStreamTest.testUploadWithConcurrentServerCloseClosesStream()
#10431
Conversation
This requires more investigations as stopping the connector should release any pending buffer not yet handed over to user code. |
36be6fc
to
56dbe8a
Compare
5bf4077
to
d3de838
Compare
@sbordet nudge |
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/AbstractHttpTest.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ludovic Orban <[email protected]>
…onClose() Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
fb96bb9
to
e290596
Compare
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
e290596
to
dd3d6d7
Compare
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
dd3d6d7
to
c4f4446
Compare
On hold until #10277 is sorted. |
The problem is that stopping the connector is notifying the HttpConnection via onClose, but that notification isn't reported to the channel state. The tentative fix of releasing the buffer in onClose and ensuring the buffer only gets released once by making it atomic isn't right as HttpConnection's buffer shouldn't need any thread safety. |
@sbordet nudge |
Signed-off-by: Ludovic Orban <[email protected]>
…10226-testUploadWithConcurrentServerCloseClosesStream
Signed-off-by: Ludovic Orban <[email protected]>
...y-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/ServerFCGIConnection.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
Outdated
Show resolved
Hide resolved
…10226-testUploadWithConcurrentServerCloseClosesStream
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything holding back this from being merged?
...t-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java
Show resolved
Hide resolved
Signed-off-by: Ludovic Orban <[email protected]>
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
Outdated
Show resolved
Hide resolved
...y-fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/internal/ServerFCGIConnection.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditional approval... with one question
LOG.debug("onClose {} stream={}", this, _stream); | ||
|
||
// If the channel doesn't have a stream, then no action is needed. | ||
if (_stream == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would _request == null
be a better test. Do we want an EofException if a connection is opened then closed without a request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the _stream == null
test for consistency with onFailure()
that tests for that too, but in practice it's identical to _request == null
as both are reset to null at the same place in recycle()
.
And I do believe it is correct to fail with a EofException
when a connection is opened then immediately closed without sending a byte.
@sbordet Do you agree?
Depends on #10432 - so this PR is a branch of that other PR.
See #10226