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.

Sign up for GitHub

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

Jump to bottom

Make idle timeouts transient #10234

Closed
gregw opened this issue Aug 6, 2023 · 6 comments · Fixed by #10844
Closed

Make idle timeouts transient #10234

gregw opened this issue Aug 6, 2023 · 6 comments · Fixed by #10844
Assignees
@lorban
Labels
Enhancement High Priority

Comments

@gregw
Copy link
Contributor

gregw commented Aug 6, 2023

Jetty version(s)
12

Enhancement Description

Idle timeouts delivered via read() should be transient, i.e. another read can be attempted after the timeout of the read chunk is not last
@gregw
Copy link
Contributor Author

gregw commented Aug 6, 2023

After work on #10233, it became apparent that AsyncContentProducer is not able to handle transient timeouts, as the error chunk is consumed in an isReady call and then null returned on the following read. This implementation needs to be modified to ensure error chunks are delivered to application before being released.

@gregw
Copy link
Contributor Author

gregw commented Aug 6, 2023

It may be that writes cannot have transient timeouts, as it will be unclear how much data, if any, was written.

Perhaps it is not worth doing transient timeouts for just reads? In which case this issue is to remove all the TODOs added in #10233

@gregw
Copy link
Contributor Author

gregw commented Aug 7, 2023

@sbordet @lorban Can we make a time to review this later this week or early next. I don't want this to get punted. I we should do this quickly or remove the TODOs and stay as is.

@sbordet sbordet mentioned this issue Aug 9, 2023
Closed
2 tasks
@gregw
Copy link
Contributor Author

gregw commented Aug 28, 2023

@lorban I think we should move this to 12.0.2

@lorban
Copy link
Contributor

lorban commented Aug 28, 2023

@gregw ack

@joakime joakime linked a pull request Sep 13, 2023 that will close this issue
Initial work to review read failures. #10278
Closed
@joakime joakime removed this from Jetty 12.0.3 Oct 11, 2023
@joakime joakime moved this to 🆕 New in Jetty 12.0.4 Oct 12, 2023
@joakime joakime removed the status in Jetty 12.0.4 Oct 12, 2023
@joakime joakime moved this to 🏗 In progress in Jetty 12.0.4 - FROZEN Oct 12, 2023
gregw added a commit that referenced this issue Nov 1, 2023
@gregw
Transient timeouts. Fixes #10234 and #10277
18c25bd
@gregw gregw mentioned this issue Nov 1, 2023
Merged
gregw added a commit that referenced this issue Nov 1, 2023
@gregw
Transient timeouts. Fixes #10234 and #10277
203babb
gregw added a commit that referenced this issue Nov 1, 2023
@gregw
Transient timeouts. Fixes #10234 and #10277
872be35
@joakime joakime linked a pull request Nov 1, 2023 that will close this issue
Fix jetty 12.0.x transient timeouts #10844
Merged
gregw added a commit that referenced this issue Nov 7, 2023
@gregw
Transient timeouts. Fixes #10234 and #10277
d901e55 
ongoing review
sbordet added a commit that referenced this issue Nov 23, 2023
@gregw @lorban
@olamy @joakime @chadlwilson @sbordet
Fix jetty 12.0.x transient timeouts (#10844)
7dcab84 
Fixes #10234

* Introduced transient failures in reads where a failure chunk has last=false.
* Transient failure now do not fail the handler callback.
* Improve eeN ContentProducer to more carefully assert transient and terminal errors + enable HttpInputIntegrationTest
* Do not add connection: close to the response when the error is transient
* Rework ChunksContentSource to support null chunks
* Added tests to verify the new transient failure cases
* Review all code that handles failure, and handling correctly transient failure, either by making them fatal, and/or by failing Content.Source.

Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Olivier Lamy <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Co-authored-by: Ludovic Orban <[email protected]>
Co-authored-by: Olivier Lamy <[email protected]>
Co-authored-by: Joakim Erdfelt <[email protected]>
Co-authored-by: Chad Wilson <[email protected]>
Co-authored-by: Simone Bordet <[email protected]>
@sbordet
Copy link
Contributor

sbordet commented Nov 28, 2023

Fixed by #10844 and further refined by #10920.

@sbordet sbordet closed this as completed Nov 28, 2023
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.4 - FROZEN Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lorban lorban

Labels
Enhancement High Priority
Projects
No open projects
Jetty 12.0.4 - FROZEN
Status: ✅ Done
Milestone
No milestone
3 participants
@gregw @sbordet @lorban