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 complete3 #4409

Merged
merged 41 commits into from
Dec 19, 2019

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Dec 10, 2019

This replaces #4398 as a fix for #4331

This PR breaks out the HttpOutput state into api state and close state.

gregw added 18 commits December 3, 2019 09:08
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]>
Cleanups of write

Signed-off-by: Greg Wilkins <[email protected]>
Signed-off-by: Greg Wilkins <[email protected]>
Work in progress

Signed-off-by: Greg Wilkins <[email protected]>
Added async close to HttpWriter and ResponseWriter
Always use async close, with blocker if necessary.

Signed-off-by: Greg Wilkins <[email protected]>
Working async close complete!

Signed-off-by: Greg Wilkins <[email protected]>
invert test as we can now call complete when not ready!

Signed-off-by: Greg Wilkins <[email protected]>
fixed transition to ERROR state

Signed-off-by: Greg Wilkins <[email protected]>
async close after onError

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

Signed-off-by: Greg Wilkins <[email protected]>
Fix for proxy tests

Signed-off-by: Greg Wilkins <[email protected]>
Fix write loop to handle clear of p=0,l=0 rather than p=l

Signed-off-by: Greg Wilkins <[email protected]>
Removed old close on all content mechanism
Cleanups and some more TODOs

Signed-off-by: Greg Wilkins <[email protected]>
a reworking of HttpOutput to separate out API state.

Signed-off-by: Greg Wilkins <[email protected]>
Soft close for Dispatcher
release buffer in onWriteComplete

Signed-off-by: Greg Wilkins <[email protected]>
Set _onError in onWriteComplete
NOOP callback instead of null

Signed-off-by: Greg Wilkins <[email protected]>
failure closes HttpOutput

Signed-off-by: Greg Wilkins <[email protected]>
Moved closedCallback handling to onWriteComplete

Signed-off-by: Greg Wilkins <[email protected]>
Additional test of complete during blocking write.

Signed-off-by: Greg Wilkins <[email protected]>
@gregw gregw marked this pull request as ready for review December 11, 2019 04:09
reimplemented blocking close to sometimes be async

Signed-off-by: Greg Wilkins <[email protected]>
ascii "art"

Signed-off-by: Greg Wilkins <[email protected]>
Code cleanup.  Use a CLOSE state rather than non null closedCallback to be clearer that it is a state.
Renamed close(Callback) to complete(Callback)
Renamed and simplified closed() to completed()

Signed-off-by: Greg Wilkins <[email protected]>
Do not dispatch
Better ascii art
improved close impl to be similar to complete

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

Signed-off-by: Greg Wilkins <[email protected]>
retain execute behaviour in 9.4. review in 10.

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

gregw commented Dec 13, 2019

@lachlan-roberts @sbordet This is ready for proper review. Would be good to get a green tick early Monday so we can start a release cycle.

Switch to CLOSING state as soon as last write is done, even if several non last channelWrites will be done.   This allows a subsequent call to close to know that nothing needs to be written and can avoid some EOF exceptions. Now onWriteComplete acts only on the passed in last parameter.

Added test for sendContent
Aggregate within lock
pipeline test debug
@gregw
Copy link
Contributor Author

gregw commented Dec 16, 2019

@sbordet @lachlan-roberts I'm am getting the failure in the attached log very occassionally... only one one laptop... only once in about 1000 runs... but sometimes 10 times in a row!!!
I have no idea what is going on at the moment, so would definitely appreciate a debugging duck at some stage. But can either of you reproduce on your machines on this branch?
DelayedServerTest_testPipeline.txt

@gregw
Copy link
Contributor Author

gregw commented Dec 16, 2019

@sbordet @lachlan-roberts I think I found the problem... I was calling the closedCallback and then releasing the buffer. I've inverted that order and now it does not fail.

Signed-off-by: Greg Wilkins <[email protected]>
Better javadoc
refactored onWriteComplete logic to be simpler
fixed bug with flush of last written byte

Signed-off-by: Greg Wilkins <[email protected]>
Completely reworked test harness for better coverage.

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

gregw commented Dec 18, 2019

92% code coverage of HttpOutput with current tests

@gregw
Copy link
Contributor Author

gregw commented Dec 18, 2019

@sbordet review please

Reworked order of ifs to match logic above in onWriteComplete

Signed-off-by: Greg Wilkins <[email protected]>
@gregw gregw merged commit c5acf96 into jetty-9.4.x Dec 19, 2019
@gregw gregw deleted the jetty-9.4.x-4331-asyncCloseComplete3 branch December 19, 2019 01:17
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.

3 participants