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

Jetty 9.4.x 4331 async close complete #4378

Closed
wants to merge 12 commits into from

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Nov 29, 2019

Fix for #4331 Implement async close and/or complete when PENDING or UNREADY.
This includes PR #4377

gregw added 10 commits November 29, 2019 10:16
Added test harness to reproduce unready completing write.
Fixed test by not closing output prior to becoming READY

Signed-off-by: Greg Wilkins <[email protected]>
Test harness to reproduce unready when closing/completing.

Signed-off-by: Greg Wilkins <[email protected]>
test both PENDING and UNREADY

Signed-off-by: Greg Wilkins <[email protected]>
test cleanups

Signed-off-by: Greg Wilkins <[email protected]>
ERROR state still needs to be closed!

Signed-off-by: Greg Wilkins <[email protected]>
close after last blocking write

Signed-off-by: Greg Wilkins <[email protected]>
If completion has to do a flush, then we need a call to closed to
avoid leaking buffers.

Signed-off-by: Greg Wilkins <[email protected]>
WIP - moved the outstate from HttpOutput to HttpChannelState
Error handling needs a big review

Signed-off-by: Greg Wilkins <[email protected]>
@gregw
Copy link
Contributor Author

gregw commented Nov 30, 2019

@sbordet, @lachlan-roberts. I would not mind a little bit of review as I work through this PR... to make sure I don't commit too much effort in a wrong direction.
So far I have:

  • merged in the pending PR Issue #4376 Async Content Complete #4377 (would be good to get that one merged)
  • added test cases for Improve handling of HttpOutput.close() for pending writes #4331 to AsyncCompletionTest
  • begun a major refactor of HttpOutput so that it's state is kept in HttpChannelState rather than separately. This will eventually allow us to deal with completion and close under a single atomic lock rather than having two locks. So far I like how it has simplified many write methods, but error handling is a mess and I'm dubious about some of locations that I callback into the state... for example onWriteComplete feels a little wrong... but it is difficult because a single API write may result in many actual writes.... In Jetty-11 we are going to have to split HttpChannelState into a core protocol state and an API state.

gregw added 2 commits December 3, 2019 07:36
Reformat

Signed-off-by: Greg Wilkins <[email protected]>
@gregw
Copy link
Contributor Author

gregw commented Dec 2, 2019

Closing this as a dead end. After review with @sbordet it was felt that moving HttpOutput.State into HttpChannelState was a wrong direction an that the HttpOutput should know it's own API state. However there were some good cleanups, so we need to extract some ideas into a new branch and PR.

@gregw gregw closed this Dec 2, 2019
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.

1 participant