Differences in calling WriteListener.onError

[lachlan-roberts]

If an async servlet is in a loop like:

    ServletOutputStream out = response.getOutputStream();
    out.setWriteListener(new WriteListener()
    {
        public void onWritePossible()
        {
            try
            {
                while (out.isReady())
                    out.write(someBuffer);
             }
             catch(Throwable t)
             {
                 t.printStackTrace();
             }     
        }

        public void onError(Throwable t)
        {
            t.printStackTrace();
        }
    });

and the client receiving the response closes the connection at some point, then we see different behaviors from Jetty, Tomcat and Undertow:

• Jetty throws from write and calls onError with a different exception. If isReady() is called it returns true.
• Tomcat throws from write and calls onError with the cause of the original exception. If isReady() is called it returns false.
• Undertow throws from the write, but never calls onError. If isReady() is called it return false, but then onError is still not called.

I'm not sure any of these behaviors is strictly correct:

• should onError be called for an exception that has already been thrown to the application?
• should onError every be called if isReady() has not been called and has not returned false?
• should the write every actually throw? instead should isReady() return false and then onError is called with the problem?
• if the write has thrown, what should isReady() return?

[gregw]

@markt-asf @stuartwdouglas I'm not sure what the correct behavior is for this? Your thoughts?

[markt-asf]

I don't see a reason for onError() to depend on whether isReady() has been called. onError() should be triggered as early as practical. I'd expect isReady() to return false once isError() has been triggered. Generally, I think applications should implement AsyncListener. If the spec clarified that read/write never threw exceptions for non-blocking I/O and that applications should always use onError() that effectively means applications would be required to implement AsyncListener. Do we want to require that? I think we need to answer that question before we can decide on whether read/write and/or onError() see the exception.

[gregw]

@markt-asf there is a reason to think that isready is involved. The sync contract is that you will get a callback IFF isready had been called and returned false. That callback can be OWP or writeListener.onError.

Note that there is also AsyncListener.onError that can be used to report errors in between Io operations. But Io operations errors should go to the write listener at least, maybe both.

@lachlan-roberts can you test if AsyncListener.onError is also being called?

Perhaps the solution is that Write listener.onError is called IFF there is a pending callback from isReady, otherwise AsyncListener.onError is called.. unless exception can be thrown from write, flush or close?

Complex! But at least we can be precise about where an exception is reported and it will only be reported once

[stuartwdouglas]

Thinking about it I think that async write should never throw, and the result should always be passed to onError(). My reasoning is that say you pass a 10mb bufffer to write(), this will pretty much always need to be written out asynchronously, so there is always a possibility that the async write will fail after the write() operation, and onError has to be called. If we allow write() to throw then there are two possible ways this failure could be reported, which IMHO is not great.

[gregw]

@stuartwdouglas I'm OK with that interpretation (that write should not throw), but then there are still some issues:

• which onError should be called? (or both?)
• what if a method like close() is called after the problematic write? should it now throw or should that fail silently and onError subsequently be called?
• what if complete() is called? Should we ever callback an event handler after complete?

[stuartwdouglas]

• The WriteListener one should definitely be called (otherwise what does it exist for?). I am +0 on calling both (or maybe more -0.5). If others are strongly in favour I am not completely opposed, but do think it kinda feels wrong to notify both. I need to think about it a bit more.

• In terms of close() we should clarify that this is an async close, and it should also never throw. Consider my example above, you can call write() with 10mb then immediately call close() while there is still data being written out. We could say that you can call close() only when isReady is true but that is problematic as isReady can be true when data is buffered but not written. The alternative would be to say that close() is blocking, but that means we no longer have a true async API.

• On something semi related we should clarify the flush() behaviour in async mode, which I interpret to be 'send all buffered data you have to the client in an async manner'

• If complete is called then IMHO we should still call onError, say I call write() with my 10mb buffer then immediately call complete(), if we don't call onError we will have no way of knowing if it fails. In general our async API has no way of notifying the code that everything has actually been sent. Even if you wait until the next callback and isReady is true there could still be data buffered internally (its just that now the implementation is ready for more data, e.g. if you are working with 8k buffers and the user attempts to write 12k you might write 8k to the socket, buffer 4k, and call onWritePossible to try and fill the remaining 4k before sending).

[gregw]

• I'm generally -0 for reporting an exception more than once. We should throw it, report it in WL.onError or report it in AL.onError, but not 2 of these and definitely not 3. Currently Jetty probably does 2 or more of these in some circumstances because it's was just not clear which we should do.

• agree that we should clarify that close() is asynchronous, and so is flush(). Ultimately both are equivalent to a write as a write can cause a flush and/or a close (if content-length is set). I think we should document that getting isReady()==true is best practice before calling close() or flush() but agree that it would break lots of apps if we tried to enforce that.

• agree that we should say that async flush() is just an async write of any buffered data from previous writes.

• I guess calling onError after a call to complete() is OK... after all complete() itself is kind of an asynchronous call as the 10GB buffer from the last write may take some time to flush. So long as we call WL.onError before calling AL.onComplete, I think we are OK.

So how about this, in async mode:

• mode, write, flush & close should never throw an IOException.
• if the IO operation associated with a write, flush or close fails, then it will be reported via the WL.onError callback, regardless of any call/return to isReady
• if an IO operation has failed, then isReady() will return false. It will continue to return false even after onError is called.
• if WL.onError is called, then that will cancel any pending callback of WL.onDataAvailable due to a false return from isReady.
• if there is an IO exception when there is no pending write, flush or close operation, then that is reported via AL.onError

[stuartwdouglas]

With regards to 'if there is an IO exception when there is no pending write, flush or close operation, then that is reported via AL.onError' I don't really like the thread safety implications, as you can basically be busy doing work that is not IO related at all, and then get an error callback running in another thread.

I guess this is no different to what happens already with timeouts, but I don't like that either :-)

[gregw]

I know what you mean. It is ugly having 3 onError methods. But if a stream is reset whilst there are no IO operations, then is an exception delivered to the WL, RL or AL?

Picking one is going to be intrinsically racey. Maybe the solution is to always deliver it to all of them?

[markt-asf]

For that specific instance I think I'd expect the exception on the AL with the WL and RL only seeing exceptions if further write/reads are attempted. ie treat it in a similar manner to timeout.

[stuartwdouglas]

Another alternative would be to delay until IO is attempted, which would match HTTP/1 semantics. That said I don't really like that approach, so I think delivering to the AL is probably the way to go.

[gregw]

So if I'm reading you both right.... in async mode:

• mode, write, flush & close may throw an Exception either for the current operation they are called for or for a previous exception if they are called after a previous exception reported either via AL.onError or WL.onError.
• if the IO operation associated with a write, flush or close (that didn't throw) fails, AND isReady() has not been called, then the exception is reported via AL.onError.
• if the IO operation associated with a write, flush or close (that didn't throw) fails, AND isReady() has been called (and returned false), then the exception is reported via WL.onError and WL.onDataAvailable will never be called.
• if an IO operation has failed, then isReady() will return false. It will continue to return false even after either onError is called.

I have quibbles with this, but no more or less than other suggestions. I don't think there is a perfect solution, so happy to go with a common OK solution.

[stuartwdouglas]

I was thinking a bit different:

So if I'm reading you both right.... in async mode:

* mode, write, flush & close may throw an Exception either for the current operation they are called for or for a previous exception if they are called after a previous exception reported either via AL.onError or WL.onError.

IMHO write, flush and close should never throw, we should have one place to handle errors and that should be the listener.

* if the IO operation associated with a write, flush or close (that didn't throw) fails, AND isReady() has not been called, then the exception is reported via AL.onError.

IMHO these should always be reported to the relevant write listener. In this case because we know the issue is related to a specific write operation then delivering to the write listener error handler makes sense. The isReady stipulation really does not make sense for errors in close().

* if the IO operation associated with a write, flush or close (that didn't throw) fails, AND isReady() has been called (and returned false), then the exception is reported via WL.onError and WL.onDataAvailable will never be called.

I think you mean onWritePossible, I agree except for the requirement to call isReady.

* if an IO operation has failed, then isReady() will return false. It will continue to return false even after either onError is called.

Makes sense.

I have quibbles with this, but no more or less than other suggestions. I don't think there is a perfect solution, so happy to go with a common OK solution.

[gregw]

IMHO write, flush and close should never throw, we should have one place to handle errors and that should be the listener.

yeah but no!!

There are two listeners we can report errors to, so there is no "the listener". I don't think it ever makes sense to always report to AL.onError (else why have WL.onError), but then it also doesn't work out to always report on WL.onError (but perhaps more sense???).

I too would like write/flush/close to never throw in async. Perhaps that can be achieved with always calling WL.onError?

Anyway, is my first proposal (3 days ago) better for you? If not, want to have a go at writing up what you think?

[stuartwdouglas]

I think the one from 3 days ago is good. I don't super like the idea of IO exceptions being delivered with no IO operations active but I think it is ok, and is really no different to what happens with timeouts.

[gregw]

This is from 3 days ago, but with the addition that write can throw if it is called after a previously reported exception.

In async mode:

• mode, write, flush & close should only throw IFF a previous exception has been reported by a call to WL.onError or AL.onError.
• if the IO operation associated with a write, flush or close fails, then it will be reported via the WL.onError callback, regardless of any call/return to isReady
• if an IO operation has failed, then isReady() will return false. It will continue to return false even after onError is called.
• if WL.onError is called, then that will cancel any pending callback of WL.onDataAvailable due to a false return from isReady.
• if there is an IO exception when there is no pending write, flush or close operation, then that is reported via AL.onError

[stuartwdouglas]

It looks like there is no circumstances where RL.onError can ever be called, due to the last point about it being reported through AL.onError.

I would say that if isReady on the ServletInputStream has returned false, the stream is still open (i.e. close has not been called and read() has not returned -1), and there is no pending write, flush or close operations then we should deliver to RL.onError.

I also don't like that as written if you attempt to read from the underlying socket, get an IO exception on the read, and there is a pending write operation then you will have to deliver the read exception to the write listener.

How about:

Write:

• Write, flush & close should only throw IF a previous exception has been reported by a call to WL.onError or AL.onError. (Note that as isReady will always return false write should throw as per the normal isReady semantics).
• If the IO operation associated with a write, flush or close fails, then it will be reported via the WL.onError callback, regardless of any call/return to isReady
• If an IO operation has failed, then isReady() will return false. It will continue to return false even after onError is called.
• If WL.onError is called, then that will cancel any pending callback of WL.onDataAvailable due to a false return from isReady.

Read:

• read and close should never throw, if invoking read would cause an exception to be reported then isReady should return false, and onError invoked.
• A read is considered to be active if isReady has returned false.
• If a read is active then any form of IOException at the transport layer should result in a delivery to RL.onError. Note that if a write is also active then this may also be delivered to WL.onError as per the rules above.

General:

• If there is an IO exception and according to the rules above it is not delivered to the RL or WL, then it is delivered to the AL. Reasons this could occur include:
• A HTTP/2 RST stream
• Background IO operations detecting a transport layer failure (e.g. the container may be buffering the request in the background, to be ready for a read)
• This potential detection of connection failure is delivered on a best effort basis, applications should not rely on it, as containers will only be able to provide it in specific circumstances.

[markt-asf]

I like a lot of this latest description. A couple of minor comments.

In Tomcat, an IOException during SIS.isReady will cause isReady to return false and RL.onError to be called. I think this is consistent with what is written above.

Read needs the same clarification that calling read or close if isReady has returned false will throw as per the standard isReady semantics. Generally, can we make the text for read and write as symmetrical as possible? At the moment it is hard to tell if the differences between them are differences in intended behaviour or just different phrasing.

[stuartwdouglas]

I think the Tomcat behavior you describe is consistent with what I have written above, even though it is not explicitly called out.

I don't think we need to clarify that read will throw after isReady returns false, as that is already part of the isReady contract. Also I don't think an InputStream close() should ever really throw. There has been no historical requirement to have isReady return true and IMHO I don't think it makes sense.

In terms of symmetry the text above was a bit of a brain dump. I think we are close to agreement so do you want me to write up a PR tomorrow and we can move the conversation there?

[markt-asf]

+1 to a PR,

[lachlan-roberts]

I have run the same test with also adding an AsyncListener directly before adding the WriteListener.

AsyncContext asyncContext = req.startAsync();
asyncContext.addListener(new AsyncListener(){...});
ServletOutputStream out = response.getOutputStream();
out.setWriteListener(new WriteListener()
{
    public void onWritePossible()
    {
        try
        {
            while (out.isReady())
                out.write(someBuffer);
         }
         catch(Throwable t)
         {
             t.printStackTrace();
         }     
    }

    public void onError(Throwable t)
    {
        t.printStackTrace();
        asyncContext.complete();
    }
});

• Jetty throws from write and calls onError with a different exception. If isReady() is called it returns true. The AsyncListener.onComplete() method is called.

• Tomcat throws from write and calls onError with the cause of the original exception. If isReady() is called it returns false. The AsyncListener methods onError() and onComplete() are both called.

• Undertow throws from the write, but never calls onError. If isReady() is called it return false, but then onError is still not called. No AsyncListener methods are ever called.

Note that I have added a asyncContext.complete() call to WriteListener.onError(). Without this Jetty will never call AsyncListener.onComplete(), however it does not change the results of Tomcat and Undertow.

[stuartwdouglas]

Undertow will call the onError method if isReady returns false and then the connection fails while the data is being written out by the IO thread in the background. My thinking was that the exception should only be reported once, but the down side of the current behavior is you have to handle it in two different spots.

The version of Undertow we use in Quarkus (which is basically a fork of Undertow on top of Vert.x) will behave much more like the proposed spec text, with methods never throwing and all errors reported via the listeners.

[lachlan-roberts]

I have repeated this experiment after updating my test over Jetty/Tomcat/Undertow to what I believe are the latest servlet 6 versions. (Jetty-12.0.5, Tomcat-10.1.18, Undertow-2.3.10.Final)

The test is setup like this:

The client is continuously reading but aborts when it reads over 1024 * 1024 * 128 bytes. The server is continuously writing in a loop inside WriteListener.onWritePossible():

try
{
    while (outputStream.isReady())
    {
        outputStream.write(data.getBytes());
    }
}
catch (Throwable t)
{
    t.printStackTrace();
}

The results:

1. Throwing from ServletOutputStream.write() inside onWritePossible().

   • Both Jetty and Undertow will always throw from the ServletOutputStream.write().
   • Tomcat will only sometimes throw, if it doesn't throw it will still calls onError after ServletOutputStream.isReady() returns false.

2. The result of ServletOutputStream.isReady() after throwing from ServletOutputStream.write().

   • Jetty will return true, while Tomcat and Undertow will return false.

3. When is WriteListener.onError() and AsyncListener.onError() called?

   • Jetty will always only call WriteListener.onError().
   • Tomcat will call both WriteListener.onError() then AsyncListener.onError().
   • Undertow will call neither.

4. When is AsyncListener.onComplete() called?

   • Always called for Jetty and Tomcat, never called for Undertow.

Note: Even if the exception from ServletOutputStream.write() is caught and not re-thrown and outputStream.isReady() is not called, then Jetty and Tomcat still invoke the WriteListener.onError().

[gregw]

Thanks @lachlan-roberts. I'll comment a bit here about what think should be the behaviour and then discuss it in more detail in the PR review.

I see no problem with ServletOutputStream.write() throwing even when in async mode. The reason being that a call to WriteListener.onError should only ever be done if there has been a prior call to isReady() that returns false. It is entirely valid for isReady() to return true and then for a subsequent call to write to throw. I think all of the following are valid sequences:

1. call to isReady() returns true
2. call to write(...) throws
3. call to isReady() returns true
4. call to write(...) throws
5. etc.

OR

1. call to isReady() returns true
2. call to write(...) returns normally
3. call to isReady() returns false
4. WriteListener.onError called
5. call to isReady() returns true
6. call to write(...) throws

I think that AsyncListener.onError(Throwable) should only be called if onWritePossible() throws, i.e. if it does not catch an exception thrown by write(...) or throws some other exception, then that will be passed to AsyncListener.onError(Throwable). But any exception passed to WriteListener.onError(Throwable) or thrown by write(...) but caught by the application, should not be passed to AsyncListener.onError(Throwable).

I think the following sequence is not valid:

1. call to isReady() returns true
2. call to write(...) returns normally
3. call to isReady() returns true
4. WriteListener.onError called

In short:

• if isReady() returns true, then write may be called, which may throw.
• if isReady() returns false, then write must not be called and either WL.onWritePossible() or WL.onError(Throwable) will eventually be called.