Bug 59219 - AsyncListener#onError not called on exception during async processing
Summary: AsyncListener#onError not called on exception during async processing
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 8.0.32
Hardware: PC All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2016-03-23 13:15 UTC by Scott Nicklous
Modified: 2016-04-25 12:58 UTC (History)
0 users

Unit test of bug 59219 (2.64 KB, patch)
2016-04-20 12:51 UTC, Abdessamed MANSOURI
Details | Diff
Unit test of bug 59219 (5.53 KB, patch)
2016-04-21 10:09 UTC, Abdessamed MANSOURI
Details | Diff
Unit test of bug 59219 (3.22 KB, patch)
2016-04-21 10:15 UTC, Abdessamed MANSOURI
Details | Diff
Unit test of bug 59219 (3.36 KB, patch)
2016-04-23 01:39 UTC, Abdessamed MANSOURI
Details | Diff
Unit test of bug 59219 (3.44 KB, patch)
2016-04-25 12:58 UTC, Abdessamed MANSOURI
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Scott Nicklous 2016-03-23 13:15:48 UTC
There are a couple of problems relating to error handling during exception
processing. Basically, Tomcat should ensure that the AsyncListener onError
and onComplete methods are correctly called if an exception occurs during
async processing in order to allow the listener code to release resources
and do clean-up. This does not seem to happen.

A sample servlet demonstrating the problem, AsyncDebugListener, is provided 
at the following location:


Build the AsyncDebugListener module using 'mvn install' and deploy it on Tomcat.

The service method and each of the AsyncListener methods logs output to the
AsyncListener.log file. You can follow execution by looking at the logs.

Assuming Tomcat is installed locally, you can use the following URLs:

1) No error - works correctly:


2) Exception during service method


If async processing is started and an exception is thrown before the end of
the service method, AsyncListener#onError is not called, however onTimeout
followed by onComplete are both called. 

This is incorrect, as onError followed by onComplete should be called. 

3) Exception during processing of AsyncContext#dispatch() target


Precondition: Async processing is started and the request is dispatched using 
AsyncContext#dispatch(). The service method returns to the container.

If the container performs the resulting async dispatch and an exception is 
thrown, Tomcat drops the connection to the browser and no AsyncListener method
is called.

This is incorrect. Tomcat should flush the contents of the buffer to the 
browser and call the AsyncListener onError followed by the onComplete methods.

Thanks for having a look at this!
Comment 1 Mark Thomas 2016-04-18 19:59:23 UTC
I've made some changes in 9.0.x to address point 2. The provided test case (thanks for that it makes life a lot easier) now passes. I want to look at point 3 before thinking about back-porting.
Comment 2 Mark Thomas 2016-04-18 20:36:05 UTC
Point 3 is fixed as well in 9.0.x for 9.0.0.M5 onwards.

I'll look at back-porting this next.
Comment 3 Mark Thomas 2016-04-18 21:08:07 UTC
Backports complete.
- 8.5.x for 8.5.1 onwards
- 8.0.x for 8.0.34 onwards
- 7.0.x for 7.0.70 onwards
- 6.0.x for 6.0.46 onwards
Comment 4 Konstantin Kolinko 2016-04-18 21:44:34 UTC
(In reply to Mark Thomas from comment #3)
> - 6.0.x for 6.0.46 onwards

Scratch the above one.
Tomcat 6 was not affected by this fix.

AsyncListener is a feature of Servlet 3.0 API / Tomcat 7 onwards.
Comment 5 Abdessamed MANSOURI 2016-04-20 12:51:34 UTC
Created attachment 33782 [details]
Unit test of bug 59219

Unit test patch of the bug 59219 (point 2).
Comment 6 Abdessamed MANSOURI 2016-04-21 10:09:52 UTC
Created attachment 33783 [details]
Unit test of bug 59219

This unit test checks 2 and 3 points.
Comment 7 Abdessamed MANSOURI 2016-04-21 10:15:13 UTC
Created attachment 33784 [details]
Unit test of bug 59219

Unit test which checks point 2 and 3.
Comment 8 Mark Thomas 2016-04-21 18:08:15 UTC
Thanks for the patch. It is being looked at. Currently I'm seeing odd behaviour on Windows so I want to test it on another platform.

I do have some basic feedback at this point. Expect some more when once I have test test working:
1. Use separate methods for separate tests. It makes it easier to determine which tests are failing.
2. We aim to keep the code warning free. If you use Eclipse, checkout the configuration settings we use under res/ide-support/eclipse
3. It looks like you should be able to re-use the existing tracking listener.
Comment 9 Mark Thomas 2016-04-21 19:25:20 UTC
And some more:
4. The wait loop logic is wrong.
   - It will always wait 5s even if the output is correct
   - If output in incorrect it will enter an infinite loop
5. It is clearer to define an explicit Wrapper insatnce and then call
   setAsyncSupported(true) on that.
6. The test doesn't pass. This actually highlights a bug in the fix (regression
   in the fix for point 2 when the fix for point 3 was made) which just goes to
   demonstrate the usefulness of test cases.

I have all this fixed locally but I think it would be useful for you to try and fix these issues yourself.
Comment 10 Mark Thomas 2016-04-22 13:04:01 UTC
I've fixed the regression. I held back on committing the test case to give you a chance to update your proposed patch.
Comment 11 Abdessamed MANSOURI 2016-04-23 01:39:40 UTC
Created attachment 33796 [details]
Unit test of bug 59219

Thank you for your useful feedback and especialy your patience and your time :), i use IntelliJ IDEA Community edition because i'm more familiar with its debugger and its features, i run ant to convert the project to eclipse project then import it inside IDEA as eclipse project (ant buildfile doesn't offer support for IDEA and IDEA supports importing eclipse projects).

The test case pass for point 2, but it doesn't for point 3, i noticed that when an exception occurs without a dispatch the onError run once, but if the exception occurs with no matter numbder of dispatch the onError run twice, i think it a bug in the fix as onError should run only once or the same as the dispatch has been called (i didn't read the servlet spec).
Comment 12 Mark Thomas 2016-04-24 16:13:26 UTC
Comment on attachment 33796 [details]
Unit test of bug 59219

Nice. That is very, very close to what I have. The differences are:
- refactoring to remove duplicate code
- setting the timeout on the async context to speed up some failure modes
- different logic around loop tracking

I'll get this committed shortly so you can see the differences. You'll get a credit in the commit message.
Comment 13 Abdessamed MANSOURI 2016-04-25 12:58:06 UTC
Created attachment 33802 [details]
Unit test of bug 59219

Inserting timeout.

About refactoring, of course there's some duplicate code as the servlets, ServletB can do what ServletA do, but i think like its better, because its more clear and more explicit for point 2, or not?? :)