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

Fixes HttpClient Content.Source reads from arbitrary threads #12203

Merged
merged 8 commits into from
Aug 30, 2024

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Aug 26, 2024

No description provided.

* Introduced a boolean parameter to parseAndFill() and parse(), that specifies whether to notify the application demand callback.
  This is necessary because reads may happen from any threads, and must not notify the application demand callback.
  Only when there is no data, and fill interest is set, then the application demand callback must be notified.
* Removed action field to avoid lambda allocation.
* Now the application is called directly from the parse() method.
* Reading -1 from the network drives the parser by calling again parse(), rather than the parser directly.
  This allows to have a central place to notify the response success event.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
…response.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
…ntent.

This avoids the rare case where the response arrives before the request thread has modified the request state, even if the request has been fully sent over the network, causing the request to be failed even if it should not.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from lorban August 26, 2024 17:06
@sbordet sbordet marked this pull request as draft August 26, 2024 17:06
@lorban
Copy link
Contributor

lorban commented Aug 27, 2024

Is that an alternative for the fix in #12143?

Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

I don't mind this change, as long as the SerializedInvoker assertions added in #12143 don't trip. So I think they should be added as part of this PR too.

Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

This is much cleaner than #12143. After modifying FCGI too, I can't insist enough that the SerializedInvoker assertions should be added too.

Copy link
Contributor

Choose a reason for hiding this comment

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

This class should have the SerializedInvoker assertions of #12143

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet marked this pull request as ready for review August 28, 2024 14:32
@sbordet sbordet requested a review from lorban August 28, 2024 14:32
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban
Copy link
Contributor

lorban commented Aug 29, 2024

@sbordet nudge

@sbordet sbordet merged commit 1726c87 into jetty-12.0.x Aug 30, 2024
8 of 10 checks passed
@sbordet sbordet deleted the fix/jetty-12.0.x/naked-content-source-reads branch August 30, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants