On Mon, Dec 12, 2022 at 4:43 PM <ma...@apache.org> wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> markt pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new 2d931eed6c Add support for Servlet read/write via ByteBuffer
> 2d931eed6c is described below
>
> commit 2d931eed6c35702ef6541c0fd6c067b79e5eb371
> Author: Mark Thomas <ma...@apache.org>
> AuthorDate: Mon Dec 12 15:43:28 2022 +0000
>
> Add support for Servlet read/write via ByteBuffer
> ---
> java/jakarta/servlet/ServletInputStream.java | 141
> ++++++++++++++++++++-
> java/jakarta/servlet/ServletOutputStream.java | 101 ++++++++++++++-
> .../catalina/connector/CoyoteInputStream.java | 14 +-
> .../catalina/connector/CoyoteOutputStream.java | 6 +
> .../org/apache/catalina/connector/InputBuffer.java | 4 +
> .../catalina/connector/LocalStrings.properties | 1 +
> java/org/apache/coyote/LocalStrings.properties | 1 -
> java/org/apache/coyote/Response.java | 5 +-
> .../apache/coyote/http11/Http11OutputBuffer.java | 7 +
> webapps/docs/changelog.xml | 11 ++
> 10 files changed, 264 insertions(+), 27 deletions(-)
>
> diff --git a/java/jakarta/servlet/ServletInputStream.java
> b/java/jakarta/servlet/ServletInputStream.java
> index 0c8280c2e9..10048fbb88 100644
> --- a/java/jakarta/servlet/ServletInputStream.java
> +++ b/java/jakarta/servlet/ServletInputStream.java
> @@ -18,6 +18,8 @@ package jakarta.servlet;
>
> import java.io.IOException;
> import java.io.InputStream;
> +import java.nio.ByteBuffer;
> +import java.util.Objects;
>
> /**
> * Provides an input stream for reading binary data from a client request,
> @@ -43,6 +45,80 @@ public abstract class ServletInputStream extends
> InputStream {
> // NOOP
> }
>
> + /**
> + * Reads from the input stream into the given buffer.
> + * <p>
> + * If the input stream is in non-blocking mode, before each invocation of
> + * this method {@link #isReady()} must be called and must return
> + * {@code true} or the {@link ReadListener#onDataAvailable()} call back
> must
> + * indicate that data is available to read else an
> + * {@link IllegalStateException} must be thrown.
> + * <p>
> + * Otherwise, if this method is called when {@code buffer} has no space
> + * remaining, the method returns {@code 0} immediately and {@code
> buffer} is
> + * unchanged.
> + * <p>
> + * If the input stream is in blocking mode and {@code buffer} has space
> + * remaining, this method blocks until at least one byte has been read,
> end
> + * of stream is reached or an exception is thrown.
> + * <p>
> + * Returns the number of bytes read or {@code -1} if the end of stream is
> + * reached without reading any data.
> + * <p>
> + * When the method returns, and if data has been read, the buffer's
> position
> + * will be unchanged from the value when passed to this method and the
> limit
> + * will be the position incremented by the number of bytes read.
> + * <p>
> + * Subclasses are strongly encouraged to override this method and
> provide a
> + * more efficient implementation.
> + *
> + * @param buffer The buffer into which the data is read.
> + *
> + * @return The number of bytes read or {@code -1} if the end of the
> stream
> + * has been reached.
> + *
> + * @exception IllegalStateException If the input stream is in
> non-blocking
> + * mode and this method is called without first calling {@link
> #isReady()}
> + * and that method has returned {@code true} or
> + * {@link ReadListener#onDataAvailable()} has not signalled that data is
> + * available to read.
> + *
> + * @exception IOException If data cannot be read for any reason other
> than
> + * the end of stream being reached, the input stream has been closed or
> if
> + * some other I/O error occurs.
> + *
> + * @exception NullPointerException If buffer is null.
> + *
> + * @since Servlet 6.1
> + */
> + public int read(ByteBuffer buffer) throws IOException {
> + Objects.requireNonNull(buffer);
> +
> + if (!isReady()) {
> + throw new IllegalStateException();
> + }
> +
> + if (buffer.remaining() == 0) {
> + return 0;
> + }
> +
> + byte[] b = new byte[buffer.remaining()];
> +
> + int result = read(b);
> + if (result == -1) {
> + return -1;
> + }
> +
> + int position = buffer.position();
> +
> + buffer.put(b, 0, result);
> +
> + buffer.position(position);
> + buffer.limit(position + result);
> +
> + return result;
> + }
> +
> /**
> * Reads the input stream, one line at a time. Starting at an offset,
> reads
> * bytes into an array, until it reads a certain number of bytes or
> reaches
> @@ -50,6 +126,8 @@ public abstract class ServletInputStream extends
> InputStream {
> * <p>
> * This method returns -1 if it reaches the end of the input stream
> before
> * reading the maximum number of bytes.
> + * <p>
> + * This method may only be used when the input stream is in blocking
> mode.
> *
> * @param b
> * an array of bytes into which data is read
> @@ -60,6 +138,10 @@ public abstract class ServletInputStream extends
> InputStream {
> * an integer specifying the maximum number of bytes to read
> * @return an integer specifying the actual number of bytes read, or -1
> if
> * the end of the stream is reached
> + *
> + * @exception IllegalStateException
> + * If this method is called when the input stream is in
> + * non-blocking mode.
> * @exception IOException
> * if an input or output exception has occurred
> */
> @@ -91,12 +173,25 @@ public abstract class ServletInputStream extends
> InputStream {
> public abstract boolean isFinished();
>
> /**
> - * Can data be read from this InputStream without blocking?
> - * Returns If this method is called and returns false, the container
> will
> - * invoke {@link ReadListener#onDataAvailable()} when data is available.
> + * Returns {@code true} if it is allowable to call a {@code read()}
> method.
> + * In blocking mode, this method will always return {@code true}, but a
> + * subsequent call to a {@code read()} method may block awaiting data. In
> + * non-blocking mode this method may return {@code false}, in which case
> it
> + * is illegal to call a {@code read()} method and an
> + * {@link IllegalStateException} MUST be thrown. When
> + * {@link ReadListener#onDataAvailable()} is called, a call to this
> method
> + * that returned {@code true} is implicit.
> + * <p>
> + * If this method returns {@code false} and a {@link ReadListener} has
> been
> + * set via {@link #setReadListener(ReadListener)}, then the container
> will
> + * subsequently invoke {@link ReadListener#onDataAvailable()} (or
> + * {@link ReadListener#onAllDataRead()}) once data (or EOF) has become
> + * available. Other than the initial call
> + * {@link ReadListener#onDataAvailable()} will only be called if and
> only if
> + * this method is called and returns false.
> *
> - * @return <code>true</code> if data can be read without blocking, else
> - * <code>false</code>
> + * @return {@code true} if data can be obtained without blocking,
> otherwise
> + * returns {@code false}.
> *
> * @since Servlet 3.1
> */
> @@ -118,4 +213,40 @@ public abstract class ServletInputStream extends
> InputStream {
> * @since Servlet 3.1
> */
> public abstract void setReadListener(ReadListener listener);
> +
> + /**
> + * {@inheritDoc}
> + * <p>
> + * This method may only be used when the input stream is in blocking
> mode.
> + *
> + * @exception IllegalStateException If this method is called when the
> input stream is in non-blocking mode.
> + */
> + @Override
> + public byte[] readAllBytes() throws IOException {
> + return super.readAllBytes();
> + }
> +
> + /**
> + * {@inheritDoc}
> + * <p>
> + * This method may only be used when the input stream is in blocking
> mode.
> + *
> + * @exception IllegalStateException If this method is called when the
> input stream is in non-blocking mode.
> + */
> + @Override
> + public byte[] readNBytes(int len) throws IOException {
> + return super.readNBytes(len);
> + }
> +
> + /**
> + * {@inheritDoc}
> + * <p>
> + * This method may only be used when the input stream is in blocking
> mode.
> + *
> + * @exception IllegalStateException If this method is called when the
> input stream is in non-blocking mode.
> + */
> + @Override
> + public int readNBytes(byte[] b, int off, int len) throws IOException {
> + return super.readNBytes(b, off, len);
> + }
> }
> diff --git a/java/jakarta/servlet/ServletOutputStream.java
> b/java/jakarta/servlet/ServletOutputStream.java
> index 86a57c6602..66ee5c1503 100644
> --- a/java/jakarta/servlet/ServletOutputStream.java
> +++ b/java/jakarta/servlet/ServletOutputStream.java
> @@ -19,7 +19,9 @@ package jakarta.servlet;
> import java.io.CharConversionException;
> import java.io.IOException;
> import java.io.OutputStream;
> +import java.nio.ByteBuffer;
> import java.text.MessageFormat;
> +import java.util.Objects;
> import java.util.ResourceBundle;
>
> /**
> @@ -45,6 +47,69 @@ public abstract class ServletOutputStream extends
> OutputStream {
> // NOOP
> }
>
> + /**
> + * Writes from the given buffer to the output stream.
> + * <p>
> + * If the output steam is in non-blocking mode, before each invocation of
> + * this method {@link #isReady()} must be called and must return
> + * {@code true} or the {@link WriteListener#onWritePossible()} call back
> + * must indicate that data may be written else an
> + * {@link IllegalStateException} must be thrown.
> + * <p>
> + * Otherwise, if this method is called when {@code buffer} has no data
> + * remaining, the method returns immediately and {@code buffer} is
> + * unchanged.
> + * <p>
> + * If the output stream is in non-blocking mode, neither the position,
> limit
> + * nor content of the buffer passed to this method may be modified until
> a
> + * subsequent call to {@link #isReady()} returns true or the
> + * {@link WriteListener#onWritePossible()} call back indicates data may
> be
> + * written again. At this point the buffer's limit will be unchanged from
> + * the value when passed to this method and the position will be the
> same as
> + * the limit.
> + * <p>
> + * If the output stream is in blocking mode and {@code buffer} has space
> + * remaining, this method blocks until all the remaining data in the
> buffer
> + * has been written. When the method returns, and if data has been
> written,
> + * the buffer's limit will be unchanged from the value when passed to
> this
> + * method and the position will be the same as the limit.
> + * <p>
> + * Subclasses are strongly encouraged to override this method and
> provide a
> + * more efficient implementation.
> + *
> + * @param buffer The buffer from which the data is written.
> + *
> + * @exception IllegalStateException If the output stream is in
> non-blocking
> + * mode and this method is called without first calling {@link
> #isReady()}
> + * and that method has returned {@code true} or
> + * {@link WriteListener#onWritePossible()} has not signalled that data
> may
> + * be written.
> + *
> + * @exception IOException If the output stream has been closed or if some
> + * other I/O error occurs.
> + *
> + * @exception NullPointerException If buffer is null.
> + *
> + * @since Servlet 6.1
> + */
> + public void write(ByteBuffer buffer) throws IOException {
> + Objects.requireNonNull(buffer);
> +
> + if (!isReady()) {
> + throw new IllegalStateException();
> + }
> +
> + if (buffer.remaining() == 0) {
> + return;
> + }
> +
> + byte[] b = new byte[buffer.remaining()];
> +
> + buffer.get(b);
> +
> + write(b);
> + }
> +
> /**
> * Writes a <code>String</code> to the client, without a carriage
> * return-line feed (CRLF) character at the end.
> @@ -278,13 +343,24 @@ public abstract class ServletOutputStream extends
> OutputStream {
> }
>
> /**
> - * Checks if a non-blocking write will succeed. If this returns
> - * <code>false</code>, it will cause a callback to
> - * {@link WriteListener#onWritePossible()} when the buffer has emptied.
> If
> - * this method returns <code>false</code> no further data must be written
> - * until the container calls {@link WriteListener#onWritePossible()}.
> + * Returns {@code true} if it is allowable to call any method that may
> write
> + * data (e.g. {@code write()}, {@code print()} or {@code flush}). In
> + * blocking mode, this method will always return {@code true}, but a
> + * subsequent call to a method that writes data may block. In
> non-blocking
> + * mode this method may return {@code false}, in which case it is
> illegal to
> + * call a method that writes data and an {@link IllegalStateException}
> MUST
> + * be thrown. When {@link WriteListener#onWritePossible()} is called, a
> call
> + * to this method that returned {@code true} is implicit.
> + * <p>
> + * If this method returns {@code false} and a {@link WriteListener} has
> been
> + * set via {@link #setWriteListener(WriteListener)}, then container will
> + * subsequently invoke {@link WriteListener#onWritePossible()} once a
> write
> + * operation becomes possible without blocking. Other than the initial
> call,
> + * {@link WriteListener#onWritePossible()} will only be called if and
> only
> + * if this method is called and returns false.
> *
> - * @return <code>true</code> if data can be written, else
> <code>false</code>
> + * @return {@code true} if data can be written without blocking,
> otherwise
> + * returns {@code false}.
> *
> * @since Servlet 3.1
> */
> @@ -306,4 +382,17 @@ public abstract class ServletOutputStream extends
> OutputStream {
> * @since Servlet 3.1
> */
> public abstract void setWriteListener(jakarta.servlet.WriteListener
> listener);
> +
> + /**
> + * {@inheritDoc}
> + * <p>
> + * If this method is called when the output stream is in non-blocking
> mode, it will immediately return with the stream
> + * effectively closed, even if the stream contains buffered data that is
> yet to be written to client. The container will
> + * write this data out in the background. If this process fails the
> {@link WriteListener#onError(Throwable)} method will
> + * be invoked as normal.
> + */
> + @Override
> + public void close() throws IOException {
> + super.close();
> + }
> }
> diff --git a/java/org/apache/catalina/connector/CoyoteInputStream.java
> b/java/org/apache/catalina/connector/CoyoteInputStream.java
> index ac3ce2d13c..01cc429903 100644
> --- a/java/org/apache/catalina/connector/CoyoteInputStream.java
> +++ b/java/org/apache/catalina/connector/CoyoteInputStream.java
> @@ -21,6 +21,7 @@ import java.nio.ByteBuffer;
> import java.security.AccessController;
> import java.security.PrivilegedActionException;
> import java.security.PrivilegedExceptionAction;
> +import java.util.Objects;
>
> import jakarta.servlet.ReadListener;
> import jakarta.servlet.ServletInputStream;
> @@ -134,18 +135,9 @@ public class CoyoteInputStream extends
> ServletInputStream {
> }
>
>
> - /**
> - * Transfers bytes from the buffer to the specified ByteBuffer. After the
> - * operation the position of the ByteBuffer will be returned to the one
> - * before the operation, the limit will be the position incremented by
> - * the number of the transferred bytes.
> - *
> - * @param b the ByteBuffer into which bytes are to be written.
> - * @return an integer specifying the actual number of bytes read, or -1
> if
> - * the end of the stream is reached
> - * @throws IOException if an input or output exception has occurred
> - */
> + @Override
> public int read(final ByteBuffer b) throws IOException {
> + Objects.requireNonNull(b);
> checkNonBlockingRead();
>
> if (SecurityUtil.isPackageProtectionEnabled()) {
> diff --git a/java/org/apache/catalina/connector/CoyoteOutputStream.java
> b/java/org/apache/catalina/connector/CoyoteOutputStream.java
> index ee005387b3..218fb00aaf 100644
> --- a/java/org/apache/catalina/connector/CoyoteOutputStream.java
> +++ b/java/org/apache/catalina/connector/CoyoteOutputStream.java
> @@ -18,6 +18,7 @@ package org.apache.catalina.connector;
>
> import java.io.IOException;
> import java.nio.ByteBuffer;
> +import java.util.Objects;
>
> import jakarta.servlet.ServletOutputStream;
> import jakarta.servlet.WriteListener;
> @@ -100,8 +101,13 @@ public class CoyoteOutputStream extends
> ServletOutputStream {
> }
>
>
> + @Override
> public void write(ByteBuffer from) throws IOException {
> + Objects.requireNonNull(from);
> boolean nonBlocking = checkNonBlockingWrite();
> + if (from.remaining() == 0) {
> + return;
> + }
> ob.write(from);
> if (nonBlocking) {
> checkRegisterForWrite();
> diff --git a/java/org/apache/catalina/connector/InputBuffer.java
> b/java/org/apache/catalina/connector/InputBuffer.java
> index ac1b816207..8d1f5f97c5 100644
> --- a/java/org/apache/catalina/connector/InputBuffer.java
> +++ b/java/org/apache/catalina/connector/InputBuffer.java
> @@ -359,6 +359,10 @@ public class InputBuffer extends Reader
> public int read(ByteBuffer to) throws IOException {
> throwIfClosed();
>
> + if (to.remaining() == 0) {
> + return 0;
> + }
> +
> if (checkByteBufferEof()) {
> return -1;
> }
> diff --git a/java/org/apache/catalina/connector/LocalStrings.properties
> b/java/org/apache/catalina/connector/LocalStrings.properties
> index 6726b4a303..09a1ed58f1 100644
> --- a/java/org/apache/catalina/connector/LocalStrings.properties
> +++ b/java/org/apache/catalina/connector/LocalStrings.properties
> @@ -37,6 +37,7 @@ coyoteConnector.protocolHandlerStopFailed=Protocol handler
> stop failed
> coyoteInputStream.nbNotready=In non-blocking mode you may not read from the
> ServletInputStream until the previous read has completed and isReady()
> returns true
> coyoteInputStream.null=The input buffer object has been recycled and is no
> longer associated with this facade
>
> +coyoteInputStream.blockingOnly=This method may not be called when the input
> stream is in non-blocking mode (i.e. after a ReadListener has been configured)
> coyoteOutputStream.nbNotready=In non-blocking mode you may not write to the
> ServletOutputStream until the previous write has completed and isReady()
> returns true
> coyoteOutputStream.null=The output buffer object has been recycled and is no
> longer associated with this facade
>
> diff --git a/java/org/apache/coyote/LocalStrings.properties
> b/java/org/apache/coyote/LocalStrings.properties
> index b708730f45..e06ffcd9a1 100644
> --- a/java/org/apache/coyote/LocalStrings.properties
> +++ b/java/org/apache/coyote/LocalStrings.properties
> @@ -65,6 +65,5 @@ request.readListenerSet=The non-blocking read listener has
> already been set
> response.encoding.invalid=The encoding [{0}] is not recognised by the JRE
> response.noTrailers.notSupported=A trailer fields supplier may not be set
> for this response. Either the underlying protocol does not support trailer
> fields or the protocol requires that the supplier is set before the response
> is committed
> response.notAsync=It is only valid to switch to non-blocking IO within async
> processing or HTTP upgrade processing
> -response.notNonBlocking=It is invalid to call isReady() when the response
> has not been put into non-blocking mode
> response.nullWriteListener=The listener passed to setWriteListener() may not
> be null
> response.writeListenerSet=The non-blocking write listener has already been
> set
> diff --git a/java/org/apache/coyote/Response.java
> b/java/org/apache/coyote/Response.java
> index b3bd46022c..fadc3fc1c1 100644
> --- a/java/org/apache/coyote/Response.java
> +++ b/java/org/apache/coyote/Response.java
> @@ -746,10 +746,7 @@ public final class Response {
>
> public boolean isReady() {
> if (listener == null) {
> - if (log.isDebugEnabled()) {
> - log.debug(sm.getString("response.notNonBlocking"));
> - }
> - return false;
> + return true;
> }
> // Assume write is not possible
> boolean ready = false;
Yeah, it's probably good to flip this, but it's not really supposed to
be used in the first place.
> diff --git a/java/org/apache/coyote/http11/Http11OutputBuffer.java
> b/java/org/apache/coyote/http11/Http11OutputBuffer.java
> index cf666b605d..570b90c0d7 100644
> --- a/java/org/apache/coyote/http11/Http11OutputBuffer.java
> +++ b/java/org/apache/coyote/http11/Http11OutputBuffer.java
> @@ -565,6 +565,13 @@ public class Http11OutputBuffer implements
> HttpOutputBuffer {
>
> @Override
> public void end() throws IOException {
> + /*
> + * TODO
> + * As of Servlet 6.1, this flush is (currently) meant to be
> + * non-blocking if the output stream is in non-blocking mode.
> That
> + * requirement creates various complications I want to discuss
> with
> + * the EG before I try implementing it.
> + */
> socketWrapper.flush(true);
> }
Ok there are the main points:
- Flushing shouldn't be used at all in non blocking, since data will
always be sent ASAP. So if you flush in the app, then it will have to
break the API promise (not blocking). Overall, if flush is allowed to
be called in non blocking, then it should be a noop instead since it
will never do anything useful (if the app wants to know when writing
is done, then it should wait for the notification).
- If (I think that's the case here) the app server blocks to wait for
the end of the request without anything from the app on the call
stack, then it possibly degrades scalability a bit but it's not
actually going to be a problem in the real world.
Remy
>
> diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
> index 02755378c3..854f21011d 100644
> --- a/webapps/docs/changelog.xml
> +++ b/webapps/docs/changelog.xml
> @@ -105,6 +105,17 @@
> issues do not "pop up" wrt. others).
> -->
> <section name="Tomcat 11.0.0-M2 (markt)" rtext="in development">
> + <subsection name="Catalina">
> + <changelog>
> + <add>
> + Update the <code>ServletInputStream</code> and
> + <code>ServletOuputStream</code> classes in the Servlet API to align
> with
> + the recent updates in the Jakarta Servlet specification to support
> + reading and writing with <code>ByteBuffer</code>s. The changes also
> + clarified various aspects of the Servlet non-blocking API. (markt)
> + </add>
> + </changelog>
> + </subsection>
> <subsection name="Coyote">
> <changelog>
> <fix>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
