Clarify behaviour after ServletOutputStream.setWriteListener

[gregw]

The current javadoc for ServletOutputStream.setWriteListener is minimal and does not well define the async behaviour, specifically in regards to which methods are affected. The 4.0 servlet specification says:

boolean isReady(). This method returns true if a write to the ServletOutputStream will succeed, 
otherwise it will return false. If this method returns true, a write operation can be performed on 
the ServletOutputStream. If no further data can be written to the ServletOutputStream. then this 
method will return false till the underlying data is flushed at which point the container will invoke 
the onWritePossible method of the WriteListener. A subsequent call to this method will return true.

As the specification is no longer available for clarifications, this behaviour needs to be codified in the javadoc and clarifications made there.
Specifically it would be desirable to define what is meant by "a write operation", as writes may be performed by methods like
ServletOutputStream.close(), ServletResponse.flushBuffers(), AsyncContext.complete(), ServletResponse.sendError(...), etc.

See also #256

[gregw]

Currently the jetty implementation interprets both close() and complete() method calls as a write operations, thus it cannot be legally called if there is a pending write operation. Specifically this make illegal code such as:

public void onWritePossible()
{
  out.write(someContent);
  out.close();
  asyncContext.complete();
}

which instead needs to be written like:

public void onWritePossible()
{
  while (out.isReady())
  {
    switch(state)
    {
      case WRITING:
        state = WRITTEN;
        out.write(someContent);
        break;
      case WRITTEN:
        state = CLOSED;
        out.close();
        break;
      case CLOSED:
        asyncContext.complete();
        return;
   }
}

The reason for this interpretation is that both operations may need to write to the network and thus may block if a write is pending. The write needed may included:

• flushing content
• sending end of chunking
• sending trailers
• TLS close handshakes

Thus if it is intended to have a fully asynchronous API, then both close and complete need to be attempted only if an isReady call has returned true. Currently in jetty, if close or complete are called with a pending write, then the response is aborted with a WritePendingException

[stuartwdouglas]

Undertow takes a more relaxed approach, by doing a non-blocking flush() or close() behind the scenes, so the first code example would work for Undertow.

My reasoning was that the purpose of isReady() is to prevent the implementation from having to buffer an unbounded amount of data, and both flush() and close() do not add any data to the buffer.

This was partly driven by our implementation choices, in the when you call write() if there is room in the buffer it will not actually attempt to write to the socket, so if you call close() there is no guarantee that all the data in the buffer can be immediately written out so we do an async close, and just call onError if there is a problem.

[gregw]

@stuartwdouglas I think undertows is also a plausible interpretation of the spec - thus why we really need to narrow it down to one behaviour and to well describe it.

Note that with undertows interpretation, some applications may be surprised that onComplete callbacks are not actually called until sometime after complete() returns, so it will fail code like:

public void onWritePossible()
{
  asyncContext.complete();
  doSomethingAfterCompletion();
}

I don't think that is very good app code, nor should we have to support it, but if the async close and complete interpretations are what we want, then we do need to javadoc it.

I think there are 3 possible interpretations: 1. async (undertow), 2. illegal (jetty), or 3. blocking (would be strange but possible). Jetty is happy to change to another behaviour if that's common to other's... @markt-asf please tell us that tomcat does 1 or 2 and not 3 :)

[gregw]

@stuartwdouglas also can you clarify how you handle something like:

public void onWritePossible()
{
  ...
  out.write(IlikeBigBuffers);
  assert(isReady()==false);
  out.close();
}

Ie if isReady() has been called and returned false, then close() is called. Is that an error for you, or do you still do an asynchronous close?
If you do the asynchronous close, do you still call onWritePossible()? If so when? immediately after initiating the close, or once the close operation itself completes (which may take some time).

Then same question for complete instead of close? I assume in this case you do not call onWritePossible() after complete has been called? However this sets up a horrid race condition as an asynchronous thread could be calling complete() just as onWritePossible() is being called, so it is going to be impossible to prevent an onWritePossible() from arriving after a complete() call. This kind of race is exactly why the 2. illegal interpretation is good - as it is just illegal to call complete() if isReady() has not returned true. Note that the same race probably exists in the case of close() but it is harder to justify an arbitrary thread calling close... but I guess it is possible.

[stuartwdouglas]

If isReady() == false then close() is not an error for us. The reason being is that close() does not add any more data to the buffer, so in this case close really means 'close after all buffered data is written'.

Once close has been called we no longer call onWritePossible().

In terms of the race I think that if you have one thread writing data asynchronously, and another thread that may call complete() at an arbitrary point your app is broken no matter what our interpretation is.

My thinking is that:

isReady() exists to provide back pressure, so the write listener cannot cause the Servlet implementation to buffer an unlimited amount of data. As both close() and flush() do not add any data to the buffer there should be no issue calling them when isReady() == false. In the case of close() this should mean 'close after all data is written', and flush() should be a noop as a flush is already in progress. In close case onException should be called as normal if something fails, otherwise the write listener should no longer be invoked.

I also think that if the user calls flush() when isReady() == true then they should be required to call isReady() again before they can write.

I really do not think that close() should block, as the write lister is generally invoked by an IO thread for performance reasons and these threads should not be blocked.

[gregw]

+1 close() should not block

+1 flush() is a write and a call to isReady() returning true is required before another write.

I'm not sure I agree that isReady() is just there to provide back pressure. It is the APIs only way to communicate that a previous async write has completed and that may be needed for other reasons within an application.

I have seen many applications where complete() is called by threads other than the one that may do the writing (probably in an onWritePossible, but not exclusively so).

Consider one async thread that is writing content and then only "looks" for more content to write if isReady() returns. It could easily write then call isReady() and get false return. Meanwhile some other async event can happen that signals no more content will be available, so the application may call complete() in another thread. If we can call complete() as an asynchronous operation in the first thread, then why not in the second? If we are saying that a complete() is not a write operations, then then is no reason for us to say it has to be in the same thread that is doing other writes? No where else have we imposed the "Same thread as" criteria, we have only imposed that a write operations should not be called until after isReady() has returned true - this may be in the same thread as called onWritePossible or it may be another... it may even be a different thread than the one that called isReady() as often isready==true might only be used to initiate some other async process to obtain more content (eg async read).

If we allow complete() to be an async call that can be called independently of isReady(), then I don't see how we can restrict which thread it is called from - or are we just restricting it to only be called after any scheduled onWritePossible callbacks? IE - I can't see how we can solve this race.

Even is we say that such races are only going to happen in bad applications, then we need to define what the behaviour should be - or at least document it.

I was kind of looking favourably at the async close and complete interpretation, but now I'm very much leaning back towards jettys just make it illegal so you don't get these race problems. If you always guard with an if(out.isReady())... then you don't have any problems.

[markt-asf]

Tomcat uses the async approach if complete() is called while an async read/write is in progress. I recently gave this code an overhaul in Tomcat. The updated state diagram that resulted might (or might not) be useful: https://github.com/apache/tomcat/blob/master/java/org/apache/coyote/AsyncStateMachine.java#L28

We generally see far more issues around error handling. I still have an in-depth review of that on my TODO list after our previous discussion on that topic.