Bug 207172 - ProjectManager.mutex vs. NbMavenProjectImpl lock ordering conflict
ProjectManager.mutex vs. NbMavenProjectImpl lock ordering conflict
Status: RESOLVED FIXED
Product: projects
Classification: Unclassified
Component: Maven
7.0.1
PC Linux
: P3 (vote)
: 7.2
Assigned To: Milos Kleint
issues@projects
: THREAD
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-11 13:12 UTC by Jan Pokorsky
Modified: 2012-06-05 06:04 UTC (History)
1 user (show)

See Also:
Issue Type: DEFECT
:


Attachments
thread dump (21.08 KB, text/plain)
2012-01-11 13:12 UTC, Jan Pokorsky
Details
proposed patch (40.67 KB, patch)
2012-05-30 08:42 UTC, Milos Kleint
Details | Diff
corrected patch (39.43 KB, patch)
2012-05-30 08:53 UTC, Milos Kleint
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Pokorsky 2012-01-11 13:12:34 UTC
Created attachment 114792 [details]
thread dump

Product Version: NetBeans IDE 7.0.1 (Build 201107282000)
Java: 1.6.0_26; Java HotSpot(TM) Client VM 20.1-b02

After closing project in Projects tab the IDE does not respond any more.
Comment 1 Jesse Glick 2012-01-11 15:02:11 UTC
"Opening projects" daemon prio=10 tid=0x09e9ec00 nid=0x131a waiting for monitor entry [0xa9cfb000]
	at org.netbeans.modules.maven.NbMavenProjectImpl$LazyLookup.beforeLookup(NbMavenProjectImpl.java:844)
	- waiting to lock <0x5d03ff18> (a org.netbeans.modules.maven.NbMavenProjectImpl)
	at org.openide.util.lookup.ProxyLookup.lookup(ProxyLookup.java:206)
	at org.netbeans.api.project.ProjectUtils.getInformation(ProjectUtils.java:101)
...
	at org.netbeans.modules.project.ui.OpenProjectList$8.run(OpenProjectList.java:776)
	at org.openide.util.Mutex.writeAccess(Mutex.java:397)


"Parsing & Indexing Loop (201107282000)" daemon prio=10 tid=0xaaddb800 nid=0xa1e in Object.wait() [0xb31fd000]
...
	at org.openide.util.Mutex.readAccess(Mutex.java:327)
	at org.netbeans.api.project.ProjectManager.findProject(ProjectManager.java:233)
...

	at org.netbeans.modules.maven.NbMavenProjectImpl.loadOriginalMavenProject(NbMavenProjectImpl.java:372)
	at org.netbeans.modules.maven.NbMavenProjectImpl.getOriginalMavenProject(NbMavenProjectImpl.java:344)
	- locked <0x5d03ff18> (a org.netbeans.modules.maven.NbMavenProjectImpl)
	at org.netbeans.modules.maven.NbMavenProjectImpl.getArtifactRelativeRepositoryPath(NbMavenProjectImpl.java:633)
Comment 2 Milos Kleint 2012-05-28 13:51:45 UTC
The only solution is to prevent the NbArtifactFixer to call FOQ->ProjectManager.

I suppose we could attempt to externalize the loading of MavenProject instances and make it possible without holding an instance of NbMavenProjectImpl..
Comment 3 Jesse Glick 2012-05-29 21:34:14 UTC
(In reply to comment #2)
> we could attempt to externalize the loading of MavenProject instances
> and make it possible without holding an instance of NbMavenProjectImpl

You mean have MFOQI.getOwnerPOM load a plain MavenProject rather than a (NB) Project, by changing getOwner(String,String,String) to return the projectDir instead of p. It would be nice, but then we would need to have some way of caching loaded MavenProject's other than as a field like NbMPI.project (and knowing when to expire the cache).

Might be simpler to address the actual lock ordering conflict shown here. Not sure what that was, since LazyLookup no longer exists - this particular deadlock may be impossible in current code.
Comment 4 Milos Kleint 2012-05-30 08:42:15 UTC
Created attachment 120050 [details]
proposed patch
Comment 5 Milos Kleint 2012-05-30 08:53:14 UTC
Created attachment 120052 [details]
corrected patch
Comment 6 Milos Kleint 2012-05-30 09:01:25 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > we could attempt to externalize the loading of MavenProject instances
> > and make it possible without holding an instance of NbMavenProjectImpl
> 
> You mean have MFOQI.getOwnerPOM load a plain MavenProject rather than a (NB)
> Project, by changing getOwner(String,String,String) to return the projectDir
> instead of p. It would be nice, but then we would need to have some way of
> caching loaded MavenProject's other than as a field like NbMPI.project (and
> knowing when to expire the cache).

to expire is fairly easy IMHO.. projects use SoftReference, when using WeakReference in the cache we should release the not belonging to project ones early and and the rest will only go when project goes.

> 
> Might be simpler to address the actual lock ordering conflict shown here. Not
> sure what that was, since LazyLookup no longer exists - this particular
> deadlock may be impossible in current code.

Well, the general problem persists. we lock on accessing the MavenProject instance and have no influence on whether someone called us without a project mutex, with a read or write one.. and then inside the project loading we access ProjectManager.findProject()..

the only reason why we don't get the deadlock more often is that expressions in GAVs are fairly rare.
Comment 7 Milos Kleint 2012-05-30 09:11:11 UTC
(In reply to comment #5)
> Created attachment 120052 [details]
> corrected patch

this is the correct patch meant for review.

1. I've removed all dependencies on projects from loading the pom. Now we load configurations in a different way when loading the pom and differently later on inside the project. A nice sideeffect was the removal of the MavenEmbedder.createModelLineage before the actual project loading.. 

2. the cache itself references MavenProject instances using WeakReference. Open question is how to reference project dir FileObjects (used as keys in the maps). Currently they are hard referenced.

3. The MavenExecutionResult instance used by the MavenProject's ProblemReporter is attached to the MavenProject instance itself, if loaded first by FOQ and later by Project, we still can populate the problemreporter

4. possible further speed improvement  in FOQ is to check if the cache contains a resolved value before executing the ModelReader


please review
Comment 8 Jesse Glick 2012-05-30 19:47:51 UTC
(In reply to comment #7)
> this is the correct patch meant for review.

BTW use the "obsoletes" option in BZ to hide older patches.

For clean patches, please delete obsolete sections of code (e.g. "} catch (RuntimeException exc) {...") rather than commenting them out.

Implementing an interface merely in order to import its constants is considered bad style; use a class with a private constructor and public static final fields, and import static them if necessary.

> how to reference project dir FileObjects (used as keys in the
> maps). Currently they are hard referenced.

This is going to be a memory leak. I think a WeakHashMap<FileObject,SoftReference<MavenProject>> would suffice for file2Project. (The MavenProject should not be strongly held since NbMPI.project holds it only softly: bug #192155.)

file2Mutex and getMutex probably do not need to exist - just synchronize on file2Project.

getMavenProject should be marked @NonNull for clarity - it is not obvious that this returns a value even for a dir which does not contain any pom.xml at all.

The rest of the patch (configurations, etc.) I do not follow well enough to review.
Comment 9 Milos Kleint 2012-06-01 08:30:54 UTC
(In reply to comment #8)
> 
> Implementing an interface merely in order to import its constants is considered
> bad style; use a class with a private constructor and public static final
> fields, and import static them if necessary.

done.

> 
> > how to reference project dir FileObjects (used as keys in the
> > maps). Currently they are hard referenced.
> 
> This is going to be a memory leak. I think a
> WeakHashMap<FileObject,SoftReference<MavenProject>> would suffice for
> file2Project. (The MavenProject should not be strongly held since NbMPI.project
> holds it only softly: bug #192155.)

Well, as long as the Project instance is around (which hard references the project's FileObject) WeakReference is as good as SoftReference for referencing the MavenProject. WeakMapping the FileObject might free the instances referenced via NBArtifactFixer early on, we only use the FO locally in a method, so unless the FileObject is held elsewhere the Map entry will be GCed via the WeakMap path. Here the unknown for me is how are FileObject instances referenced and de-referenced.

> 
> file2Mutex and getMutex probably do not need to exist - just synchronize on
> file2Project.

That's going to create a bottleneck. Previously the synchronization was done on per-Project basis, now it would be global.

> 
> getMavenProject should be marked @NonNull for clarity - it is not obvious that
> this returns a value even for a dir which does not contain any pom.xml at all.
> 

I've fixed that by returning null early on to prevent having the cache flooded with fallback poms for non-existing projects.
Comment 10 Milos Kleint 2012-06-01 11:13:24 UTC
http://hg.netbeans.org/core-main/rev/d6b1ad61ed81
Comment 11 Jesse Glick 2012-06-01 14:47:35 UTC
(In reply to comment #9)
> WeakReference is as good as SoftReference for referencing
> the MavenProject.

Agreed.

> WeakMapping the FileObject might free the instances
> referenced via NBArtifactFixer early on

Yes, it could, but this is preferable to holding irrelevant files forever. (o.n.m.projectapi.TimedWeakReference is a handy compromise; maybe it should be made a utility API?)

>> just synchronize on file2Project.
> 
> That's going to create a bottleneck.

Of course, but that is not necessarily bad - unless you really expect multiple hardware threads to be loading MavenProject's instances in parallel. Since this job is likely I/O-bound I would expect that to be slower than simple serial loading.

> returning null early on to prevent having the cache flooded
> with fallback poms for non-existing projects.

Right, that is probably better.
Comment 12 Milos Kleint 2012-06-01 14:57:44 UTC
(In reply to comment #11)
> (In reply to comment #9)
> >> just synchronize on file2Project.
> > 
> > That's going to create a bottleneck.
> 
> Of course, but that is not necessarily bad - unless you really expect multiple
> hardware threads to be loading MavenProject's instances in parallel. Since this
> job is likely I/O-bound I would expect that to be slower than simple serial
> loading.
> 

it's the scary reality that requests to load MavenProject instances come from all directions, including AWT. (FileEncodingQuery etc.). it's possible that in sum of all loading times the serial loading is faster, but will be likely slower for these critical case (IMHO). We've had non-serial loading so far, that's another reason I'm more comfortable with that approach (the known evil beats unknown one)
Comment 13 Quality Engineering 2012-06-05 06:04:12 UTC
Integrated into 'main-golden', will be available in build *201206050001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/d6b1ad61ed81
User: Milos Kleint <mkleint@netbeans.org>
Log: #207172 have MavenProject instances loaded without need for a Project, keep external cache of such instances and call it from both NbMavenProjectImpl and NbArtifactFixer, that way we get rid of entering ProjectMutex while loading the MavenProject instances (via NbArtifactFixer and MavenFileOwnerQueryImpl)


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo