Clarify under what circumstances onError is called

[stuartwdouglas]

Fixes #433

Signed-off-by: Stuart Douglas [email protected]

[stuartwdouglas]

I have ended up making some changes compared to what was discussed, as the more I thought about it I thought there were some issues with the proposals we talked about on the issue. In particular I think the issues are:

• At the moment AsyncListener.onError is only really called as a result of dispatch operations, having it occur due to underlying connection failure (potentially delivered by an IO thread) seems like a breaking change, as this method could now be called in circumstances where it was not possible before.
• ReadListener and WriteListener onError methods are now invoked when a read is attempted (generally when isReady is called). This matches the behavior of blocking mode, where you only get notified of IO failure when you attempt to perform an operation.

Basically the more I thought about it the more I came to the conclusion that if we want to support eager notification of IO failure (e.g. RST_STREAM) we should add a special listener just for this that is usable in both blocking and non blocking mode.

I also think that delivering IOException to AsyncListener on eager IO failure is also a mistake, as this is not what the method has been used for in the past.

[gregw]

Stuart,
I'll review your text in detail shortly, but I do have an issue with:

• At the moment AsyncListener.onError is only really called as a result of dispatch operations, having it occur due to underlying connection failure (potentially delivered by an IO thread) seems like a breaking change, as this method could now be called in circumstances where it was not possible before.

It is a not uncommon use case that some compute intensive job is done asynchronous, but it is desirable to stop that work if the connection is ever closed. This is an oft requested feature, but in the days of http1 was just not possible because you could not detect close without attempting IO. But this is now possible with http2 and http3. Jetty definitely has users that plug into AL.onError and extect to see some exception or other if the stream is reset, even if no IO is attempted.

Now that we have h2/h3 it seams silly to make such applications waste all that CPU finishing their compute intensive task, only to finally discover the connection has been closed when they try to write the response. We would have to put in a non-compliant mode to support such users if we end up hiding exceptions without IO. Now they don't need to be delivered to AL.onError... so long as they are delivered to one of the onError listeners in a timely fashion, then we are good.

[stuartwdouglas]

The issue I have with this approach is that you can still have that requirement even with blocking IO and without using startAsync at all. If we tie this capability into the read/write listeners (or even the AL) then we limit the applications that can make use of it.

[gregw]

Hmmm the text is rather complex and on my third read through I'm still not sure I entirely understand.

I'm wondering if we are trying to be too smart by avoiding multiple reports of the same issue. What about if whenever there is an issue with the connection, we report it once to each and any of the onError listeners registered.
I can't really think of any exceptions that are going to affect the read side, but not the write side or versa vice. If a read suffers a broken pipe exception, then we are going to have to onError the write side as well. If a h2 stream is reset, that is both read and write.

So how about:

• read/write/close/flush do not throw, unless called after any onError callback has been called.
• if an error happens on a connection or during any IO operation, then it will be reported to any WL, RL or AL registered (in that order!) and isReady will return false from that time forward.

[stuartwdouglas]

Basically something like this: #435

[stuartwdouglas]

So how about:

* read/write/close/flush do not throw, unless called after any onError callback has been called.

+1, that should already be covered by the text

* if an error happens on a connection or during any IO operation, then it will be reported to any WL, RL or AL registered (in that order!) and isReady will return false from that time forward.

I would rather deliver do both a WL and RL, as long as the relevant streams are still open (e.g. if read() has returned -1 then we should not deliver to the RL, same if close has been called on either stream).

If we can't deliver to a RL or WL then I would rather add a mechanism like in #435 that can always be notified even when async is not in use.

[gregw]

@stuartwdouglas I'm OK with not calling WL.onError or RL.onError if they have been closed, which means:

• a RL is closed if:
  • InputStream.close() has been called
  • a read has returned -1 or
  • onAllDataRead has been called
  • RL.onError has previously been called
• a WL is closed if:
  • OutputStream.close has been called
  • exactly the content length has been written (successfully)
  • WL.onError has previously been called

As for AL.onError, I'm happy for it to always be called, or if we want to avoid duplication, it could be called IFF one of the RL or WL is either not set or is closed.

[stuartwdouglas]

I agree with your description of 'RL is closed', however 'WL is closed if OutputStream.close has been called' should be 'OutputStream.close has been called and any buffered data has been successfully written to the client', as the close could trigger a write which could then fail.

Like I mentioned above I would much rather we add a connection specific listener rather than repurpose AL.onError to handle async connection failures. If everyone else is in favor I guess I can live with it but it feels wrong to me, and can change the behaviour of existing applications as it can now be called at times when it would not otherwise have been called.

[markt-asf]

The current Tomcat behaviour during async IO can be summarised as:

• any async IO errors are delivered to WL (if configured) and RL (if configured)
• If RL.isReady() triggers an IOE (this is where Tomcat does the read) it is delivered RL.onError
• If RL.onDataAvailable throws any throwable it is delivered RL.onError
• If RL.onAllDataRead throws any throwable it is delivered RL.onError
• If SOS.close or SOS.flush throws an IOE it it delivered to WL.onError
• If WL.onWritePossible throws any throwable it is delivered WL.onError
• Any throwables from the app are delivered to AL.onError

On reflection, I share Stuart's concern about starting to send async connection issues to AL. I can see the benefits of an new listener for these.

I wonder if we can simplify the "is RL/WL closed" element to "if the request is in async mode"?

I think we should include the handling of exceptions thrown from RL.onDataAvailable and friends in the list of reasons RL/WL onError will be called.

[gregw]

@stuartwdouglas Can you wake up this PR and let's get it merged.

[gregw]

updates in line with my comments on the issue #433.

I also think we should update AsyncListener to say that it's onError will be called for errors when there are no IO operations current (i.e. no ReadListener#onError and no WriteListener#onError applicable) or if an exception is thrown from onWritePossible or onDataAvailable etc.

[markt-asf]

I think I am OK with the direction this is heading in but I'd like to review the changes once the PR has been rebased to take account of the clarifications that have already been made to the affected classes.

[stuartwdouglas]

Sorry, I was on PTO, I have rebased and applied Greg's suggestions.

[stuartwdouglas]

I think this text has some problems.

Say I am streaming some data from a remote source and call write(), I am expecting more data in future, but I don't have any more data ready yet.

If the async write files in this case then we have no way of reporting this to the user until they attempt to call isReady(). Because they have not call isReady and it returned false we are not allowed to invoke the listener.

[stuartwdouglas]

I also really don't like the idea of doubling up error handling. The original proposal meant that you only had to implement onError, with this change you now need to handle errors thrown from the stream. Should we add a section that if onWritePossible throws IOException then the onError method is called? It seems like obvious behavior but I am not sure if it is called out anywhere. This would mean that if write throws you can just let the exception propagate and the listener will handle it.

[gregw]

Stuart, by onError in your comment, I'm assuming that you mean AsyncListener.onError and not WriteListener.onError.

So I do like the "if OWP throw then the AL.onError method is called" as a good way to give control to the application about how write errors are reported to AL.onError.

I agree it there is something strange about not reporting a known error to WL.onError until isReady() is called. But if we do not, then we do not have an easy way to tell if a previous write has completed or not. Currently the only way we have on knowing a previous write has completed is if isReady() has returned true. Perhaps you could also say that if onWritePossible has been called, that also indicates completion... but really isReady() should be called from within OWP and checked for a true response to protect from spurious wakeups. If WL.onError can be called at any time, then it may be called simultaneously to another thread calling isReady() and then the app will never know if the call to WL.onError was the result of the false return from isReady or if it just happened anyway and another call is on its way.

I'm not sure there is a good solution given the current API. I think the best we can do is be rigorous with the scheduling so we at least avoid races like the one above.

Note that if the app you described really wants to know about an error before the next write is ready, there is nothing stopping it calling isReady() immediately after the write and then it will know that the operation has either completed or that either ODA or WL.onError will be called as soon as the operation is complete.

[stuartwdouglas]

Thinking about it some more I agree with your concerns about thread safety. The only way I can think to make this work is to allow this use case via the flush method. We could add something like 'If flush() is called in async mode then isReady must not return true until the data is written out to the client'.

Then if you really care about error notification and are not going to immediately write again you can call flush + isReady to see the results.