Bug 52188 - DirectoryScanner is not threadsafe
Summary: DirectoryScanner is not threadsafe
Status: RESOLVED FIXED
Alias: None
Product: Ant
Classification: Unclassified
Component: Core (show other bugs)
Version: 1.8.2
Hardware: All All
: P2 critical (vote)
Target Milestone: 1.8.3
Assignee: Ant Notifications List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-15 18:23 UTC by Alex
Modified: 2012-01-21 07:07 UTC (History)
0 users



Attachments
patch for DirectoryScanner.java (4.14 KB, patch)
2011-11-15 18:35 UTC, Alex
Details | Diff
Alternative fix for thread-safety issue of default excludes (2.39 KB, patch)
2011-12-02 16:45 UTC, Stefan Bodewig
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex 2011-11-15 18:23:58 UTC

    
Comment 1 Alex 2011-11-15 18:34:55 UTC
We have a build target which runs several concurrent builds inside of a parallel block.  This target would intermittently through several NPE's and fail.  Long story short, I chased the NPE's back to the DirectoryScanner class.  In looking at the class it was clear that there several methods which modified the same member, and had no locking in place to prevent concurrent modification of that member.  

Since applying the attached patch, we have not seen this error (several hundred builds later). Whereas without the patch, we see the failure in at least 1 out of every 10 builds.
Comment 2 Alex 2011-11-15 18:35:44 UTC
Created attachment 27939 [details]
patch for DirectoryScanner.java
Comment 3 Stefan Bodewig 2011-11-17 11:17:18 UTC
Your patch synchronizes access to the default excludes - which may be a good thing to do in any case - which implies the tasks you are running in parallel are modifying the default excludes.  How can you ensure consistent results?

About your patch, I probably wouldn't make defaultExcludes volatile but rather final (and rewrite the reset method to clear the set rather than create a new one) - in particular since you are synchronizing on it.

Did you really have to make isExcluded and isSelected synchronized?  These instance methods shouldn't be used by multiple threads in your parallel scenario at all.
Comment 4 Stefan Bodewig 2011-12-02 16:23:27 UTC
While putting together an alternative patch I discovered a bug in
addDefaultExcludes.  The array length may be wrong if default exlcudes
are modified while this method executes.

I'll attach my version of the fix here once Ant's test suite is through.
Comment 5 Stefan Bodewig 2011-12-02 16:45:46 UTC
Created attachment 28018 [details]
Alternative fix for thread-safety issue of default excludes

@Adam: it would be great if you could apply my proposed alterantive and see whether any multithreading problems occur.
Comment 6 Stefan Bodewig 2011-12-02 16:46:56 UTC
> @Adam:

That should have been Alex, sorry.

Stefan
Comment 7 Alex 2011-12-08 18:51:47 UTC
Hi Stephan,

Sorry for the slow response; been very busy.  I will give your patch a shot as soon as I get some time to do it.  Hopefully sometime in the next couple of weeks. 

Thanks for the help!
Comment 8 Alex 2011-12-08 21:13:14 UTC
... sorry, I meant to say @Stefan.  I guess it's contagious :-D
Comment 9 Stefan Bodewig 2012-01-21 07:07:15 UTC
Applied my version of the patch with svn revision 1234276

Please reopen this bug if the problem persists.