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.

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

Clarify handling of IOException inside [Read|Write]Listener methods #396

Open
markt-asf opened this issue Feb 25, 2021 · 2 comments
Open

Comments

@markt-asf
Copy link
Contributor

This came up while investigating a Tomcat bug report. I'm going to refer to WriteListener.onWritePossible but it applies to ReadListener.onDataAvailable() and ReadListener.onAllDataRead() as well.

A simplified listener implementation may look something like this:

public void onWritePossible() throws IOException {
   ...
    do {
        sos.write(...);
    } while (sos.isReady());
} 

The question is:

Is the application permitted to swallow the IOException that may be thrown by sos.write(...) in the above code or must it be (re-)thrown from the method for the container code that called onWritePossible() to handle?

I can make a case for either approach. I have no strong preference (Tomcat now handles both cases) but I think it would be useful to add some clarification to the Javadoc for these methods to make the expected behaviour here explicit both for container developers and end users.

@gregw
Copy link
Contributor

gregw commented Feb 25, 2021

I don't think that a container should rely on the application to not swallow the exception. But there are some questions that would be good to clarify:

  • if the application catches an exception thrown from sos.write() and does not rethrow, does the container call onError with the original exception?
  • if the application catches an exception thrown from sos.write() and then throws a different exception, then which exception (if either) is passed in a call to onError? Does one exception suppress the other?

I had thought that onErrror would only ever be called instead of onWritePossible if an error occurred asynchronously, and that there should be no need to call it with any exception thrown from onWritePossible, but apparently that is not the case and we changed jetty some time ago to call onError in both situations.

@markt-asf
Copy link
Contributor Author

Assuming we go with the approach that the container does not rely on the application not swallowing the exception, I'd say the answers to your questions are:

  • yes, the container calls onError with the original exception
  • I'd be happy with with either but lean towards the application thrown exception simply because that is what Tomcat does now.

Tomcat also calls onError in both situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants