Bug 60385 - ServletRequestListener.requestDestroyed not invoked after exception in requestInitialized
Summary: ServletRequestListener.requestDestroyed not invoked after exception in reques...
Status: RESOLVED INVALID
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 7.0.35
Hardware: Macintosh Mac OS X 10.1
: P2 major (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-16 23:05 UTC by Todd West
Modified: 2016-11-28 15:35 UTC (History)
0 users



Attachments
Builds a WAR that reproduces the issue (52.51 KB, application/x-gzip)
2016-11-16 23:05 UTC, Todd West
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Todd West 2016-11-16 23:05:53 UTC
Created attachment 34456 [details]
Builds a WAR that reproduces the issue

If an application has a ServletRequestListener defined that throws an exception from its requestInitialized() method, it will prevent the requestDestroyed() method from ever being invoked for that request. This causes major problems when a framework relies on requestInitialized and requestDestroyed always being called at least once for every request.



Steps to Reproduce:

1) Build the attached project which generates a WAR

2) Deploy the WAR to Tomcat

3) Visit the /tomcat-listener-bug-1.0/simple page 6 times. On the 6th request the application will shut down if requestDestroyed() is never called



Actual Results:

The application shuts down.


Expected Results:

The application should stay up and requestDestroyed() should be called for every call to requestInitialized().




Build Date & Hardware:

Originally tested on Tomcat 7.0.35, Mac OS X 10.1 and Java 7. The issue occurs on all versions of Tomcat (even up to 9.0.0.M13) and all versions of Java as well.




Additional Builds and Platforms:

I tested this same sample app on Jetty 9 and Glassfish 4 (newest releases of both as of 11/16/2016) and it does not exhibit this same behavior. On both of those servers requestDestroyed() is called for every single requestInitialized(), even in the case of an exception.
Comment 1 Todd West 2016-11-16 23:12:18 UTC
For a little more information, the code involved in this issue can be found in org.apache.catalina.core.StandardHostValve. Specifically in the "invoke(Request request, Response response)" method.

When context.fireRequestInitEvent(request) is called, if it returns false (in the case of an exception from a handler) then it will exit this method without ever calling fireRequestDestroyEvent(request) to ensure that it can clean up after itself.
Comment 2 Remy Maucherat 2016-11-16 23:26:35 UTC
You're pretending to rely on behaviors that were never specified.

requestInitialized: Receives notification that a ServletRequest is about to come into scope of the web application.
requestDestroyed: Receives notification that a ServletRequest is about to go out of scope of the web application.

If there's an exception in requestInitialized, it probably doesn't come into scope as per the javadoc language, so that's probably why Tomcat implemented it this way originally. BTW, other servers do it like that as well.
Comment 3 Todd West 2016-11-16 23:38:10 UTC
I agree with your assessment of the issue but I have not been able to find another app server the behaves this way. As noted in the report above I tried in both Jetty and Glassfish and both of those servers responded how I would expect, by calling requestDestroyed.

I can probably find a way to work around this issue but I figured it was worth mentioning that Tomcat is the odd one out here as far as I've been able to tell. If this behavior is expected and deviation from other servers is ok feel free to close this out if you'd like.
Comment 4 Remy Maucherat 2016-11-16 23:55:22 UTC
JBoss EAP 6 does it like that. As I said, the other servers probably use a try/finally, but Tomcat's implementation looks to be on purpose to do what the javadoc says rather than shove the requestDestroy in the finally.
Anyway, have fun with your bug report, bye.
Comment 5 Remy Maucherat 2016-11-16 23:57:41 UTC
Fix unintended status change.
Comment 6 Mark Thomas 2016-11-25 09:59:02 UTC
There is additional specification language that supports Tomcat's implementation in the Javadoc for ServletRequestListener:
<quote>
A ServletRequest is defined as coming into scope of a web application when it is about to enter the first servlet or filter of the web application, and as going out of scope as it exits the last servlet or the first filter in the chain.
</quote>

An exception in requestInitialized() will prevent the request entering the first servlet/filter so it can never exit it.

It probably wouldn't hurt for the spec to be more explicit on the expected behaviour here. I have no particular preference on what that should be but based on what the spec currently says, I believe that Tomcat's implementation is spec compliant.
Comment 7 Todd West 2016-11-28 15:35:55 UTC
(In reply to Mark Thomas from comment #6)
> There is additional specification language that supports Tomcat's
> implementation in the Javadoc for ServletRequestListener:
> <quote>
> A ServletRequest is defined as coming into scope of a web application when
> it is about to enter the first servlet or filter of the web application, and
> as going out of scope as it exits the last servlet or the first filter in
> the chain.
> </quote>
> 
> An exception in requestInitialized() will prevent the request entering the
> first servlet/filter so it can never exit it.
> 
> It probably wouldn't hurt for the spec to be more explicit on the expected
> behaviour here. I have no particular preference on what that should be but
> based on what the spec currently says, I believe that Tomcat's
> implementation is spec compliant.


Thank you for your reply, Mark. That's definitely understandable.

At the very least I think it's great to have this information documented here so anyone else that runs into this issue can realize that it's working as intended.