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 CachingHttpContentFactory$CachedHttpContent already released buffer #12704

Merged

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Jan 13, 2025

Atomically retain the buffer with a check that it still is in the cache, otherwise delegate to the wrapped content.

Fixes #12681

…the cache, otherwise delegate to the wrapped content

Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban requested a review from gregw January 13, 2025 12:14
@lorban lorban self-assigned this Jan 13, 2025
@lorban lorban requested a review from sbordet January 13, 2025 12:15
@lorban lorban added the Bug For general bugs on Jetty side label Jan 13, 2025
@lorban lorban marked this pull request as ready for review January 13, 2025 12:15
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
@gregw
Copy link
Contributor

gregw commented Jan 13, 2025

@lorban also is there a version of this for 12.0?

gregw added a commit that referenced this pull request Jan 13, 2025
This is a partial fix for #12681. It does not fix the actual release race on a single content, but by restricting the shrinking to a single thread, make the race less likely to occur.

There is a more comprehensive fix in #12678 and #12704 for 12.1,
@gregw
Copy link
Contributor

gregw commented Jan 13, 2025

I merged this to #12678 to check it works with the demos. All good there.

@lorban
Copy link
Contributor Author

lorban commented Jan 14, 2025

@gregw I backported this fix to 12.0.x here: #12710

The logic is sightly different as 12.0 does not have a writeTo() method but exposes retain() instead. It's quite ugly but since it's 12.0 only I think it's alright.

Merging both PRs to 12.1 is going to be fun!

@lorban lorban requested review from sbordet and gregw January 14, 2025 11:06
@lorban
Copy link
Contributor Author

lorban commented Jan 14, 2025

@sbordet @gregw please come to an agreement about the private cachedRetain() method's name.

@gregw
Copy link
Contributor

gregw commented Jan 14, 2025

@sbordet @gregw please come to an agreement about the private cachedRetain() method's name.

I agree that cachedRetain() is not good. But I do think retainIfCached is a much clearer name than just retain. We have many retain methods and none of them do logic like this method, nor return a boolean.
Another alternative could be tryRetain

Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban merged commit 491b2ed into jetty-12.1.x Jan 15, 2025
2 of 4 checks passed
@lorban lorban deleted the fix/jetty-12.1.x/12681-cached-content-already-released branch January 15, 2025 09:49
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
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants