Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 322683

Summary: Threading issues in RewriteHandler
Product: [RT] Jetty Reporter: Gunnar Wagenknecht <gunnar>
Component: serverAssignee: Greg Wilkins <gregw>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: jesse.mcconnell, jetty-inbox
Version: 7.1.4   
Target Milestone: 7.1.x   
Hardware: All   
OS: All   
Whiteboard:

Description Gunnar Wagenknecht CLA 2010-08-13 15:05:09 EDT
I investigates RewriteHandler today for my use cases and while looking at its #handle implementation I think I found a threading issue.

The implementation looks like this:
325:  if (isStarted())
326:  { 
327:    String returned = _rules.matchAndApply(target, request, response);
328:    target = (returned == null) ? target : returned;
329:        
330:    if (!_rules.isHandled())
331:    {
332:      super.handle(target, baseRequest, request, response);
333:    }
334:  }

Please correct me if I'm wrong. But it's #handle called from multiple to handle multiple parallel requests? 

'_rules' is a private member and shared between threads. The threading issue happends between '_rules.matchAndApply' (line 327) and '_rules.isHandled' (line 330). "_rules.isHandled" is implemented as a boolean field in RuleContainer. It maybe changed by a different thread and thus may not reflect a different state than the intended on (set by '_rules.matchAndApply').

Is this intended or an oversight? Should RuleContainer use a ThreadLocal?
Comment 1 Jesse McConnell CLA 2010-08-13 15:28:12 EDT
Thanks Gunnar..

looking through things now

cheers,
jesse
Comment 2 Gunnar Wagenknecht CLA 2010-08-13 15:41:08 EDT
It seems that isHandle is actually just a synonym for Request#isHandled. RuleContainer only sets it together with the following code (RuleContainer.java, line 210).

(request instanceof Request?(Request)request:HttpConnection.getCurrentConnection().getRequest()).setHandled(true);

Maybe the code in RewriteHandle should check the request instead of '_rule.isHandled()' and the '_handled' logic can be removed in RuleContainer?
Comment 3 Jesse McConnell CLA 2010-08-13 15:43:28 EDT
yes, I am leaning that way as well

I think that in a nutshell this could be a source of some intermittent failures to urls that ought to be rewritten or things that were not rewritten not being passed further up the handler chain...

I think that is a nice catch :)
Comment 4 Greg Wilkins CLA 2010-08-15 22:26:31 EDT
ooops that was a bit of a howler!
Fix in process.
Comment 5 Greg Wilkins CLA 2010-08-16 00:06:10 EDT
fix applied in r2211 for jetty-7.

Not extensively tested yet.  Also need to backport to jetty-6
Comment 6 Jesse McConnell CLA 2010-10-04 18:37:04 EDT
fixed a while back, checking now to make sure it was resolved in jetty6 and if not will open an issue in jira for it