Skip to content
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 stack overflow in HttpStreamOverHttp3.consumeAvailable() #10562

Merged
merged 11 commits into from
Sep 27, 2023

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Sep 21, 2023

Only H3 seems to suffer from the stack overflow problem.

What I do not like about this "fix":

  • there is no easy way to test it
  • it exposes that all protocols leak buffers on the server when a timeout occurs

Fixes #10543

@lorban lorban added the Bug For general bugs on Jetty side label Sep 21, 2023
@lorban lorban added this to the 12.0.x milestone Sep 21, 2023
@lorban lorban requested a review from sbordet September 21, 2023 10:08
@lorban lorban self-assigned this Sep 21, 2023
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should start using SoftReference in our RetainableByteBuffer to link to the ByteBuffer. We would also have a hard reference, which we would set when the buffer is in the pool, and null when we allocate the buffer. If we ever get a buffer in the collection queue, we then know that somebody took a RetainableByteBuffer and leaked it. However, I'm not exactly sure how we would recover from that, as I'm not sure how we can use the collected ByteBuffer to find the RetainableByteBuffer reference?

@lorban
Copy link
Contributor Author

lorban commented Sep 22, 2023

@gregw Looking at the ConcurrentPool implementation, it may be possible to keep both a hard and a weak reference to the pooled object and null out the hard ref when the pooled object is acquired, then assign it back to the hard reference when it is released. We could then "garbage collect" the pool with a timer by removing all entries that have a null WeakReference.

That looks feasible and would make our pools totally resistant to leaks like they used to be. We should investigate this idea. I filed #10569 so that we can come back to that subject later on.

@lorban lorban linked an issue Sep 22, 2023 that may be closed by this pull request
…rflow

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
…owing when the stream is finished to avoid a stack overflow

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban force-pushed the jetty-12.0.x-10543-fix-stream-consumeAvailable branch from 6baa204 to 3672d7c Compare September 25, 2023 09:40
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban
Copy link
Contributor Author

lorban commented Sep 25, 2023

@sbordet I streamlined the H1 consumeAvailable impl; actually the fix for the memory leak should have been in HttpConnection.fillRequestBuffer().

But the code I added to H3 should be considered a workaround. There is something more fundamental that is wrong with the way H3 handles exceptions in the lowest layers. I'm going to open an issue about it.

@lorban lorban requested a review from sbordet September 25, 2023 16:04
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this for this release cycle, so long as we get onto #10569 asap next cycle.

… when needed

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
…ementations

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
…ementations

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban merged commit ffb00fd into jetty-12.0.x Sep 27, 2023
@lorban lorban deleted the jetty-12.0.x-10543-fix-stream-consumeAvailable branch September 27, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review HttpStream.consumeAvailable() implementations
3 participants