servlet
servlet copied to clipboard
jakartaee
Clarify under what circumstances onError is called
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.
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.
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.
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.
Basically something like this: https://github.com/eclipse-ee4j/servlet-api/pull/435
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.
@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.
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.
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.
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.