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

Improve handling of HttpOutput.close() for pending writes #4331

Closed
sbordet opened this issue Nov 19, 2019 · 2 comments
Closed

Improve handling of HttpOutput.close() for pending writes #4331

sbordet opened this issue Nov 19, 2019 · 2 comments

Comments

@sbordet
Copy link
Contributor

sbordet commented Nov 19, 2019

Jetty version
9.4.23
Java version
Any
Description
Applications, especially reactive ones, may issue a final write and then think they can call AsyncContext.complete() without waiting for the write to be complete:

public void onWritePossible() {
  // Only one buffer to write.
  httpOutput.write(byteBuffer);
  asyncContext.complete();
}

This does not really respect the Servlet 3.1 Async I/O contract although the matter is underspecified.

Would be better if applications did this:

private boolean finished;
public void onWritePossible() {
  // Only one buffer to write.
  while (httpOutput.isReady()) {
    if (finished) {
      asyncContext.complete();
    } else {
      httpOutput.write(byteBuffer);
      finished = true;
  }
}

That is, applications should always call isReady() and make sure that the call to AsyncContext.complete() happens when the write is completed.

@gregw can you clarify this with the Servlet Expert Group?

sbordet added a commit that referenced this issue Nov 19, 2019
Added test case that verifies the current behavior (abort the response
in case complete() is called with a pending write()).

Signed-off-by: Simone Bordet <[email protected]>
@gregw
Copy link
Contributor

gregw commented Nov 19, 2019

See jakartaee/servlet#273

@gregw
Copy link
Contributor

gregw commented Nov 28, 2019

In addition, review of #4377 suggests that we need to better handle the ERROR state of HttpOuput and decide if it is a terminal or transient state.

gregw added a commit that referenced this issue Nov 29, 2019
Test harness to reproduce unready when closing/completing.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Nov 29, 2019
test both PENDING and UNREADY

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Nov 29, 2019
test cleanups

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Nov 30, 2019
WIP - moved the outstate from HttpOutput to HttpChannelState
Error handling needs a big review

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 4, 2019
Test harness to reproduce unready when closing/completing.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 4, 2019
test both PENDING and UNREADY

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 4, 2019
test cleanups

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 4, 2019
Cleanups of write

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 4, 2019
Work in progress

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 5, 2019
Added async close to HttpWriter and ResponseWriter
Always use async close, with blocker if necessary.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 5, 2019
Working async close complete!

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 5, 2019
invert test as we can now call complete when not ready!

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 5, 2019
fixed transition to ERROR state

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 5, 2019
async close after onError

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 6, 2019
minor cleanups

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 6, 2019
Fix for proxy tests

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 8, 2019
Fix write loop to handle clear of p=0,l=0 rather than p=l

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 9, 2019
Removed old close on all content mechanism
Cleanups and some more TODOs

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 10, 2019
a reworking of HttpOutput to separate out API state.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 10, 2019
Soft close for Dispatcher
release buffer in onWriteComplete

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 10, 2019
Set _onError in onWriteComplete
NOOP callback instead of null

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 10, 2019
failure closes HttpOutput

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 11, 2019
Moved closedCallback handling to onWriteComplete

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 11, 2019
Additional test of complete during blocking write.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 12, 2019
reimplemented blocking close to sometimes be async

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 12, 2019
ascii "art"

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 13, 2019
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]>
gregw added a commit that referenced this issue Dec 13, 2019
Do not dispatch
Better ascii art
improved close impl to be similar to complete

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 13, 2019
More test cases

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 13, 2019
retain execute behaviour in 9.4. review in 10.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 18, 2019
Better javadoc
refactored onWriteComplete logic to be simpler
fixed bug with flush of last written byte

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 18, 2019
Completely reworked test harness for better coverage.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 18, 2019
Reworked order of ifs to match logic above in onWriteComplete

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue Dec 19, 2019
* Issue #4376 Async Content Complete

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]>

* Issue #4331 Async Write Complete

Test harness to reproduce unready when closing/completing.

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

* Issue #4331 Async Write Complete

test both PENDING and UNREADY

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

* Issue #4331 Async Write Complete

test cleanups

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

* Issue #4331 Async Close Complete

Cleanups of write

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

* WIP

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

* Issue #4331 Close Complete

Work in progress

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

* Issue #4331 Close Complete

Added async close to HttpWriter and ResponseWriter
Always use async close, with blocker if necessary.

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

* Issue #4331 Close Complete

Working async close complete!

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

* Issue #4331 Close Complete

invert test as we can now call complete when not ready!

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

* Issue #4331 Close Complete

fixed transition to ERROR state

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

* Issue #4331 Close Complete

async close after onError

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

* Issue #4331 Close Complete

minor cleanups

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

* Issue #4331 Close Complete

Fix for proxy tests

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

* Issue #4331 Close Complete

Fix write loop to handle clear of p=0,l=0 rather than p=l

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

* Issue #4331 Close Complete

Removed old close on all content mechanism
Cleanups and some more TODOs

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

* Issue #4331 Close Complete

a reworking of HttpOutput to separate out API state.

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

* Issue #4331 Close Complete

Soft close for Dispatcher
release buffer in onWriteComplete

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

* Issue #4331 Close Complete

Set _onError in onWriteComplete
NOOP callback instead of null

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

* Issue #4331 Close Complete

failure closes HttpOutput

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

* Issue #4331 Close Complete

Moved closedCallback handling to onWriteComplete

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

* Issue #4331 Close Complete

Additional test of complete during blocking write.

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

* Issue #4331 Close Complete

reimplemented blocking close to sometimes be async

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

* Issue #4331 Close Complete

ascii "art"

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

* Issue #4331 Close Complete

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]>

* Issue #4331 Close Complete

Do not dispatch
Better ascii art
improved close impl to be similar to complete

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

* Issue #4331 Close Complete

More test cases

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

* Issue #4331 Close Complete

retain execute behaviour in 9.4. review in 10.

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

* Improved javadoc and ascii art

* Improved CLOSING

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

* WIP

Aggregate within lock
pipeline test debug

* Avoid creating ignored exception when Idle or Failed.

* Try a parse without fill to avoid unconsumed input debug

* fixed pipeline size

* release buffer before callback

* turn off debug

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

* Issue #4331 Close Complete

Better javadoc
refactored onWriteComplete logic to be simpler
fixed bug with flush of last written byte

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

* Issue #4331 Close Complete

Completely reworked test harness for better coverage.

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

* Issue #4331 Close Complete

Reworked order of ifs to match logic above in onWriteComplete

Signed-off-by: Greg Wilkins <[email protected]>
@sbordet sbordet closed this as completed Dec 20, 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

No branches or pull requests

2 participants