Bug 57977

Summary: The original class loader isn't re-bound to the thread in PersistentValve.invoke()
Product: Tomcat 8 Reporter: Kyohei Nakamura <nakamura.kyohei.lab>
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: minor Keywords: PatchAvailable
Priority: P2    
Version: 8.0.x-trunk   
Target Milestone: ----   
Hardware: All   
OS: All   
Attachments: patch against trunk
patch(rev.1) against trunk.

Description Kyohei Nakamura 2015-06-01 07:40:43 UTC
Created attachment 32769 [details]
patch against trunk

In PersistentValve.invoke(Request, Response) method, The Webapp class loader has been bound to the current thread.
However, the original class loader that had been used before bind isn't restored to the thread.
The original class loader should be re-bound to the thread.
Comment 1 Christopher Schultz 2015-06-01 13:49:59 UTC
Would it be possible to revert the changes (likely whitespace) that don't affect the code at all? It would make for a much cleaner and readable diff.
Comment 2 Kyohei Nakamura 2015-06-02 05:20:10 UTC
Created attachment 32776 [details]
patch(rev.1) against trunk.

Thank you for your comment.
I attached the patch that ignored whitespace changes.
Comment 3 Christopher Schultz 2015-06-02 22:17:27 UTC
Great! That's much easier to see what you actually changed (just added a try/finally block)! It looked like you had made many more changes than that.
Comment 4 Remy Maucherat 2015-06-08 15:42:19 UTC
Actually, the patch looks wrong. If the valve is set at the host level (or below), then nothing is needed, the host valve is the one supposed to be doing the context classloader setting. If somehow the valve is located at the engine level, then this looks rather weird (but maybe it's nice if there are lots of vhosts), and in that case it needs to set the context classloader (and some more - normally any such change is supposed to be done with context.(un)bind now).

Since PersistentValve is not documented anywhere, the its use is questionable (non sticky sessions are not really allowed), I'd add a note in the javadoc saying it should be associated to a Host or Context.
Comment 5 Mark Thomas 2015-06-09 11:17:33 UTC
At the Host level the context class loaders are set in the StandardHostValve which is the basic Valve - i.e. always the last one in the chain. Therefore the clsss loader switching will be required for Hosts and Engines but not Contexts (or Wrappers although I can't think why this would ever be configued on a Wrapper).
Comment 6 Remy Maucherat 2015-06-09 11:44:42 UTC
Very good point about the host, thanks.
Comment 7 Mark Thomas 2015-06-09 11:58:22 UTC
Fixed in trunk (for 9.0.x), 8.0.x (for 8.0.24 onwards) and 7.0.x
(for 7.0.63 onwards).