jakartaee / servlet

Jakarta Servlet
https://eclipse.org/ee4j/servlet
Other
258 stars 82 forks source link

Fix issue-484 Add read/write via ByteBuffer #485

Closed markt-asf closed 1 year ago

markt-asf commented 1 year ago

Could be applied as-is but more likely to be a starting point for discussion.

The default implementations could be more efficient but I opted for clarity over efficiency given that containers should be overriding these methods anyway.

One obvious question - to we want to add scatter/gather read/write?

markt-asf commented 1 year ago

I've updated the text.

One further question occurred to me. The implement can't (easily) perform the check that would throw an ISE if the stream isn't ready for a non-blocking read/write. The options I can see are:

  1. ignore the missing check
  2. drop the implementation and make the method abstract
  3. add additional plumbing that enables us to determine if the stream is in non-blocking mode

Ignoring the check seems wrong. The additional plumbing might not be possible since any concrete implementation we add for setting the listener (so we can detect if we are in non-blocking mode) will have no effect as the container's implementation will override it. I think that leaves making these new methods abstract.

gregw commented 1 year ago

@markt-asf Can't the implementations just call isReady() and throw ISE on a false return? Not very efficient, but then it's not meant to be.

markt-asf commented 1 year ago

That would work for non-blocking mode. In blocking mode, isReady() could return false since it is defined (in the Javadoc) as "Returns true if data can be read without blocking else returns false.". That would trigger the ISE when we don't want one.

If we redefined isReady() so it always returned true when in blocking mode then yes, we could simply check isReady(). That seems reasonable to me since there is no reason apps should be calling isReady() when in blocking mode.

There is a risk that apps are doing something odd and calling isReady() when still in blocking mode but it seems low enough to me that changing the definition is acceptable. Thoughts?

gregw commented 1 year ago

I think returning true for isReady in blocking mode is fine. Perhaps saying that a true return means that it is allowed to call the API, rather than saying you can call "without blocking."... which kind of implies that you can call it, but it would block which is not the case.

markt-asf commented 1 year ago

@gregw I'll work on an update that defines isReady() returns true in the blocking case.

markt-asf commented 1 year ago

Updated with isReady() changes and added throwing ISE to read/write methods.

markt-asf commented 1 year ago

Language updated to clarify various ambiguities.

markt-asf commented 1 year ago

OK. I have updated the Javadoc to take account of most of the recent discussion.

I haven't yet changed the Javadoc for ServletOutputStream.close(). Tomcat does something different and I want to think about this use case further first.

markt-asf commented 1 year ago

Close text added with just minor changes for consistency.

markt-asf commented 1 year ago

Wording updated.