Clarify how async ReadListener changes the contract of ServletInputStream methods

[gregw]

The use of an async ReadListener allows the normal read methods to be used in async mode. However nowhere in the javadoc do we document how that behaviour is changed and I think they may be a few corner cases that are ambiguous:

• can the ServletInputStream.readLine method be used? Currently it has a default implementation that will fail in async mode as isReady() is not called. Should implementations override this with a method that works asynchronously and returns 0 if a complete line is not available? Or should the default implementation be updated to throw ISE if there is a ReadListener?
• Ditto for readAllBytes() and readNBytes(...), skip(long), skipNBytes(long), transferTo(OutputStream): should they ISE, block or return null if insufficient data is available?
• The javadoc of isReady() is very light on and really needs to make clear the scheduling implications of a false return - ie that one of the ReadListener callbacks will eventually be called if false is return.
• If read(byte[],int,int) is called without previously calling isReady():
  • Should ISE always be thrown?
  • Should ISE be thrown only if there is no data available?
  • Should 0 be returned if there is no data available? If so, is this equivalent to a call to isReady() returning false? ie will a callback be scheduled when data is available?
• If read(byte[],int,int) is called after isReady() returns false and onDataAvailable, should 0 be returned or ISE thrown?
• If read() is called, then returning 0 is not an option as it is a valid byte. So how should read() handle all of the situations above?

[gregw]

@markt-asf @stuartwdouglas any thoughts on this? Do you want me to propose some javadoc changes to clarify the way I think it should be? which are:

• ISE should be thrown from readLine, readAllBytes, readNBytes, transferTo, skipNBytes and any other blocking methods added to InputStream
• skip works normally
• ISE will be thrown by any read without a prior isReady call. You may need to talk me down from this one... but at the very least read() has to throw ISE if no data is available. If we allow 0 to be returned from a read, then it should not be the same as returning false from isReady - ie no scheduling is done.

[markt-asf]

I don't think we can't do anything about readAllBytes, readNBytes, transferTo or skipNBytes as none of them are Java 8 methods. Happy with skip working normally. I think ISE is the only realistic option for readLine I'm fine with an ISE for a read() without a prior isReady. Tomcat already does this.

[stuartwdouglas]

I am happy with the above, Undertow will already throw an ISE for read() without a prior isReady.

[joakime]

Perhaps these decisions should be reflected in new TCK tests?

[bshannon]

Perhaps these decisions should be reflected in new TCK tests?

Yes, please!

[joakime]

I don't think we can't do anything about readAllBytes, readNBytes, transferTo or skipNBytes as none of them are Java 8 methods.

Now with the recent release of the Servlet Spec on post Java 8 bytecodes, I would like to revive this discussion and make these methods also throw ISE.