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 - take 2 #4398

Closed
wants to merge 18 commits into from

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Dec 4, 2019

take two at a fix for #4331

gregw added 8 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]>
@gregw
Copy link
Contributor Author

gregw commented Dec 4, 2019

@lachlan-roberts and @sbordet as before... some review of work in progress appreciated so I don't bark up the wrong mixed metaphor

gregw added 6 commits December 5, 2019 12:52
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]>
continue;
if (LOG.isDebugEnabled())
LOG.debug("onWritePossible");
_writeListener.onWritePossible();
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are in state CLOSED or CLOSING we will now call onWritePossible() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we want to do that.... if we have got to UNREADY state we should call owp even if we are closed.
But I'm only 95% sure of that...

There are strange situations... like if we are closed then isReady should return true (else another dispatch is scheduled)...
but if we sent content length and then the very last write of the content congests for a while - what should isReady return? true because it knows that the connection will be closed once the write is complete, or false thus causing an OWP callback with a closed output! I think the later is correct... but we need to review carefully!

gregw added 2 commits December 6, 2019 16:59
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]>
@gregw gregw marked this pull request as ready for review December 8, 2019 21:50
@gregw gregw requested a review from lachlan-roberts December 8, 2019 21:51
@gregw
Copy link
Contributor Author

gregw commented Dec 8, 2019

@sbordet can you try this with cometd and the reactive API to check it doesn't break async behaviours?

gregw and others added 2 commits December 9, 2019 23:01
Removed old close on all content mechanism
Cleanups and some more TODOs

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

gregw commented Dec 10, 2019

I'm closing in favour of #4409

@gregw gregw closed this Dec 10, 2019
@joakime joakime deleted the jetty-9.4.x-4331-asyncCloseComplete2 branch January 10, 2020 00:40
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