Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify under what circumstances onError is called #434

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Clarify under what circumstances onError is called #434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7fbce3
Clarify under what circumstances onError is called
stuartwdouglas Nov 3, 2021
4936159
Apply suggestions from code review
stuartwdouglas Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion api/src/main/java/jakarta/servlet/ReadListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,20 @@ public interface ReadListener extends EventListener {
void onAllDataRead() throws IOException;

/**
* Invoked when an error occurs processing the request.
* Invoked when an error occurs reading data after {@link #setReadListener(ReadListener)} has been called. This method
* will be invoked if there is a problem while data is being read from the stream and either:
* <ul>
* <li>{@link ServletInputStream#isReady()} has been invoked and returned false;</li>
* <li>or {@link ServletInputStream#close()} has been called, and the failure occurred before the response could be
* completed</li>
* </ul>
* If these conditions are not met and the stream is still open then any failure notification will be delivered either:
* by an exception thrown from a {@code IO} operation after an invocation of {@link ServletInputStream#isReady()} has
* returned {@code true}; or by a call to this method after an invocation of {@link ServletInputStream#isReady()} has
* returned {@code false};
* <p>
* This method will not be invoked in any circumstances after {@link AsyncListener#onComplete(AsyncEvent)} has been
* called.
*
* @param t the throwable to indicate why the read operation failed
*/
Expand Down
18 changes: 17 additions & 1 deletion api/src/main/java/jakarta/servlet/ServletInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,30 @@ public int readLine(byte[] b, int off, int len) throws IOException {
* available. Other than the initial call, {@link ReadListener#onDataAvailable()} will only be called if and only if
* this method is called and returns false.
*
* If an attempt is made to read from the stream when the stream is in async mode and this method has not returned
* {@code true} the method will throw an {@link IllegalStateException}.
* <p>
* If an error occurs then either: this method will return {@code true} and an {@link IOException} will be thrown from a
* subsequent call to {@code read(...)}; or this method will return {@code false} and subsequently
* {@link ReadListener#onError(Throwable)} will be invoked with the error. Once the error has either been thrown or
* passed to {@link ReadListener#onError(Throwable)}, then this method will always return {@code true} and all further
* {@code read} operations will throw an {@link IOException}.
* <p>
*
* @return {@code true} if data can be obtained without blocking, otherwise returns {@code false}.
* @see ReadListener
* @since Servlet 3.1
*/
public abstract boolean isReady();

/**
* Instructs the <code>ServletInputStream</code> to invoke the provided {@link ReadListener} when it is possible to read
* Instructs the <code>ServletInputStream</code> to invoke the provided {@link ReadListener} when it is possible to
* read.
* <p>
* Note that after this method has been called, methods on this stream that are documented to throw {@link IOException}
* might not throw these exceptions directly, instead they may be reported via {@link ReadListener#onError(Throwable)}
* after a call to {@link #isReady()} has returned {@code false}. Please refer to {@link #isReady()} method for more
* information.
*
* @param readListener the {@link ReadListener} that should be notified when it's possible to read.
*
Expand Down
22 changes: 20 additions & 2 deletions api/src/main/java/jakarta/servlet/ServletOutputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,15 @@ public void println(double d) throws IOException {
* {@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.
* <p>
* If an attempt is made to write to the stream when the stream is in async mode and this method has not returned
* {@code true} the method will throw an {@link IllegalStateException}.
* <p>
* If an error occurs then either: this method will return {@code true} and an {@link IOException} will be thrown from a
* subsequent call to {@code write(...)}; or this method will return {@code false} and subsequently
* {@link WriteListener#onError(Throwable)} will be invoked with the error. Once the error has either been thrown or
* passed to {@link WriteListener#onError(Throwable)}, then this method will always return {@code true} and all further
* {@code write} operations will throw an {@link IOException}.
*
* @return {@code true} if data can be written without blocking, otherwise returns {@code false}.
* @see WriteListener
Expand All @@ -345,8 +354,17 @@ public void println(double d) throws IOException {

/**
* Instructs the <code>ServletOutputStream</code> to invoke the provided {@link WriteListener} when it is possible to
* write
*
* write.
* <p>
* Note that after this method has been called, methods on this stream that are documented to throw {@link IOException}
* might not throw these exceptions directly, instead they may be reported via {@link ReadListener#onError(Throwable)}
* after a call to {@link #isReady()} has returned {@code false}. Please refer to {@link #isReady()} method for more
* information.
* <p>
* Once this method has been called {@link #flush()} and {@link #close()} become asynchronous operations, possibly
* performed in the background. Thus any problems may be reported through the {@link WriteListener#onError(Throwable)}
* method, which may be called at any time after a {@link #close()} or otherwise only after a call to {@link #isReady()}
* has returned {@code false}.
*
* @param writeListener the {@link WriteListener} that should be notified when it's possible to write
*
Expand Down
16 changes: 15 additions & 1 deletion api/src/main/java/jakarta/servlet/WriteListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,21 @@ public interface WriteListener extends EventListener {
void onWritePossible() throws IOException;

/**
* Invoked when an error occurs writing data using the non-blocking APIs.
* Invoked when an error occurs writing data after {@link ServletOutputStream#setWriteListener(WriteListener)} has been
* called. This method will be invoked if there is a problem while data is being written to the stream and either:
* <ul>
* <li>{@link ServletOutputStream#isReady()} has been invoked and returned false</li>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this text has some problems.

Say I am streaming some data from a remote source and call write(), I am expecting more data in future, but I don't have any more data ready yet.

If the async write files in this case then we have no way of reporting this to the user until they attempt to call isReady(). Because they have not call isReady and it returned false we are not allowed to invoke the listener.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also really don't like the idea of doubling up error handling. The original proposal meant that you only had to implement onError, with this change you now need to handle errors thrown from the stream. Should we add a section that if onWritePossible throws IOException then the onError method is called? It seems like obvious behavior but I am not sure if it is called out anywhere. This would mean that if write throws you can just let the exception propagate and the listener will handle it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stuart, by onError in your comment, I'm assuming that you mean AsyncListener.onError and not WriteListener.onError.

So I do like the "if OWP throw then the AL.onError method is called" as a good way to give control to the application about how write errors are reported to AL.onError.

I agree it there is something strange about not reporting a known error to WL.onError until isReady() is called. But if we do not, then we do not have an easy way to tell if a previous write has completed or not. Currently the only way we have on knowing a previous write has completed is if isReady() has returned true. Perhaps you could also say that if onWritePossible has been called, that also indicates completion... but really isReady() should be called from within OWP and checked for a true response to protect from spurious wakeups. If WL.onError can be called at any time, then it may be called simultaneously to another thread calling isReady() and then the app will never know if the call to WL.onError was the result of the false return from isReady or if it just happened anyway and another call is on its way.

I'm not sure there is a good solution given the current API. I think the best we can do is be rigorous with the scheduling so we at least avoid races like the one above.

Note that if the app you described really wants to know about an error before the next write is ready, there is nothing stopping it calling isReady() immediately after the write and then it will know that the operation has either completed or that either ODA or WL.onError will be called as soon as the operation is complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it some more I agree with your concerns about thread safety. The only way I can think to make this work is to allow this use case via the flush method. We could add something like 'If flush() is called in async mode then isReady must not return true until the data is written out to the client'.

Then if you really care about error notification and are not going to immediately write again you can call flush + isReady to see the results.

* <li>{@link ServletOutputStream#close()} has been called, and the failure occurred before the response could be fully
* written to the client</li>
* </ul>
*
* If these conditions are not met and the stream is still open then any failure notification will be delivered either:
* by an exception thrown from a {@code IO} operation after an invocation of {@link ServletOutputStream#isReady()} has
* returned {@code true}; or by a call to this method after an invocation of {@link ServletOutputStream#isReady()} has
* returned {@code false};
* <p>
* This method will not be invoked in any circumstances after {@link AsyncListener#onComplete(AsyncEvent)} has been
* called.
*
* @param t the throwable to indicate why the write operation failed
*/
Expand Down
3 changes: 3 additions & 0 deletions spec/src/main/asciidoc/servlet-spec-body.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8586,6 +8586,9 @@ Jakarta Servlet {spec-version} specification developed under the Jakarta EE Work

=== Changes Since Jakarta Servlet 6.0

link:https://github.com/eclipse-ee4j/servlet-api/issues/433[Issue 433]::
Clarify how IO errors are handled by async streams.

stuartwdouglas marked this conversation as resolved.
Show resolved Hide resolved
link:https://github.com/eclipse-ee4j/servlet-api/issues/59[Issue 59]::
A new attribute, `jakarta.servlet.error.query_string`, has been added to the
attributes that must be set on the request as part of an error dispatch.
Expand Down
Loading