Porting exception propagation changes to Tomcat 3.3

[Larry Isaacs <La...@sas.com>]

Hi Costin,

I'm in the process of porting the Tomcat 3.2 exception propagation
changes to Tomcat 3.3 that were made to Handler.java,
ServletWrapper.java, and RequestDispatcherImpl.java.  I was trying
to review behavior as I do so to see what can be improved or cleaned
up.

One thing that looks like in can be removed is the synchronization
found in ServletWrapper.doInit().  If you were to sync block here,
it would mean that the preServletInit() interceptors had been
called twice.  It looks like the "init" synchronization in
Handler.service() should suffice.  Would you agree?

I was going to port the default error response changes next.  Part of
that involved adding an errorURI property to ResponseImpl.java.  It is
used to carry the "error URI" from lower levels in case it is needed by
the top level handler that outputs the default error response.  It there
a better way to transport this "error URI" that setting it on the
response? Would using a note be consistent with the design?

One more question, should ServletWrapper.init() be changed to call
postServletInit() interceptors, even if doInit() throws an exception?
If init() fails, I would think that the interceptors should be made aware
of this fact.

Thanks,
Larry

Re: Porting exception propagation changes to Tomcat 3.3

[cm...@yahoo.com]

Hi Larry,

> I'm in the process of porting the Tomcat 3.2 exception propagation
> changes to Tomcat 3.3 that were made to Handler.java,
> ServletWrapper.java, and RequestDispatcherImpl.java.  I was trying
> to review behavior as I do so to see what can be improved or cleaned
> up.

The only important change in 3.3 is the fact that most of the error
handling is now done in a module ( and can be replaced with a different
module without requiring a change in the core). 

My only wish is to have as much as possible in the module, and as little
as possible in the core ( that is good for a lot of reasons ) .


> One thing that looks like in can be removed is the synchronization
> found in ServletWrapper.doInit().  If you were to sync block here,
> it would mean that the preServletInit() interceptors had been
> called twice.  It looks like the "init" synchronization in
> Handler.service() should suffice.  Would you agree?

+1 

> I was going to port the default error response changes next.  Part of
> that involved adding an errorURI property to ResponseImpl.java.  It is
> used to carry the "error URI" from lower levels in case it is needed by
> the top level handler that outputs the default error response.  It there
> a better way to transport this "error URI" that setting it on the
> response? Would using a note be consistent with the design?

If this is a general property that will (or could) be used by other
modules, probably adding get/set is better.
 
If it is used just by few modules - probably making it a note is
better. 

I'm fine with any of those ( and we can change it later ).
 
> One more question, should ServletWrapper.init() be changed to call
> postServletInit() interceptors, even if doInit() throws an exception?
> If init() fails, I would think that the interceptors should be made aware
> of this fact.

I think postServletInit should be called - it is possible that an
interceptors creates some resources ( start a transaction ) in
preServletInit and it has to be able to release it. 

I haven't reviewed that code - I want to finish the Parameters and Headers
refactoring, and also improve/refactor MessageBytes - those are the last
big code changes, and after that I can try to work on the details of
startup/shutdown/class loaders/etc.

( BTW, you have my big +1 to change whatever you feel will improve the
code, and I would be very happy if you could take care and review all
error handling, and also for the initialization/shutdowns. I'll read all
commits and try to give as much feedback as possible )

Costin