Community
Participate
Working Groups
Although I have not detected a definite pattern, upon starting the workbench, if you try to do something right away like open a plugin.xml file or Search for a Java type, the classpath container of some projects would resolve to nothing. This indicates that the following method is return an empty array: public IClasspathEntry[] getClasspathEntries() { if (model == null) return new IClasspathEntry[0]; if (entries == null) { entries = ClasspathUtilCore.computePluginEntries(model); entries = verifyWithAttachmentManager(entries); } return entries; } So it could be that either: 1. the model is null. This would occur if PDE has not created a plug-in model for the Java project when JDT/Core has asked it to resolve the container. If PDE reacts to POST_CHANGE before JDT/Core, it would have a chance to initialize the model before JDT, and this problem would/should not occur. 2. entries = ClasspathUtilCore.computePluginEntries(model) is returning an empty array. This would mean that PDE models are not initialized. This seems unlikely because the particular model associated with this container is initialized, and all models are initialized at the same time. However, if this is the culprit scenario, it is a dangerous one since the empty array is now cached in the PDE container.
The method where the master table of plug-ins gets initialized was not thread- safe, and a flawed boolean variable was being used to avoid reentrant code. Here is what was happening: The table of plug-in models is lazily initialized. When one thread (namely the "All Types Caching" thread) enters the initializeTable() method below, it marks the model manager as locked with a variable and starts initializing models. Now another thread ("Main") enters initializeTable() to resolve the classpath container, it will exit the method silently at line 1 because workspaceManager.isLocked() will return true, so the model for that particular project would be null, and we will give back to JDT/Core an empty classpath. private void initializeTable() { if (workspaceManager == null || workspaceManager.isLocked()) return; entries = new HashMap(); // now go into the workspace model manager, mark it as locked with a variable, initialize it, mark it as unlocked and return. ..... } Fixed by marking the intializer methods as synchronized. Once a thread makes it into the method, if the hashtable containing the models is null (i.e. this is the first thread), it initializes the models. Second thread blocks when thread 1 is executing the method. Once the second thread enters the method, the hashtable is not null (since this thread was blocked until the first one finished initializing), the second thread in this case does nothing and returns. private synchronized void initializeTable() { if (workspaceManager == null || entries != null) return; entries = new TreeMap(); ..... } Before putting the change in, I was getting the problem very frequently in my runtime workbench. After the change, the problem has not reoccurred. So I will mark this as fixed and will release the code into the I-build.
Man, I hate threads :-).
If you want an extra set of eyes to look at the fix, I don't mind taking a look... I've seen my share of threading issues over the past year :) Just point me at the class...
Thank you John. Please do so. The method in question is org.eclipse.pde.internal.core.PluginModelManager.initializeTable() in the org.eclipse.pde.core plug-in.
I have a couple of recommendations: 1. Remove double-test around call to initializeTable. Say Thread 1 is inside initializeTable, and has run past the second line: private synchronized void initializeTable() { if (workspaceManager == null || entries != null) return; entries = new TreeMap(); //execution of Thread 1 is here Now say that Thread 2 calls the public getPlugins() method. Since "entries" is non-null, the first two checks are skipped, and it never enters the initializeTable method. It then proceeds to iterate over the table entries: public IPluginModelBase[] getPlugins() { if (entries == null) initializeTable(); if (entries == null) return new IPluginModelBase[0]; Collection values = entries.values();//Thread 2 gets here -> You now have Thread 1 writing to the "entries" map, and Thread 2 reading the same map. This is not allowed according to spec of TreeMap (and will thus fail in unspecified ways), and also I suspect it will cause other threads to read partially initialized models. I recommend removing the "entries==null" check and *always* calling initializeTable() prior to any access to the table (perhaps enforced by a table accessor method and avoiding all references to the field outside that accessor). 2. The "entries" map is modified during model updates, and it looks like this can happen during resource change notification. If someone reads the "entries" table while it is being updated, this is again not supported by TreeMap and could fail in mysterious ways. I recommend making "entries" thread safe unless you can guarantee that it will never be read during modification. The two traditional techniques here are either wrapping "entries" in a synchronized map (Collections.synchronizedMap), or creating a new copy of the table every time you modify it. The second option is more efficient than a synchronized map if the table is rarely modified. See for example ResourceInfo.setSessionProperty for an example of a table copy-on-write pattern. 3. I know you guys hate source code comments, but I recommend putting some source comments in whenever the code must maintain thread safety rules (i.e., "field x must never be accessed directly", "field y must never be written outside method foo", "this method is synchronized to protect structure x", etc). Threading bugs have a way of creeping back in despite best efforts otherwise. If you later decide to synchronize a different method in the same class for a different reason, you may expose yourself to unneccessary locking, deadlock, etc.
Thanks John (aka. concurrency police :-) for your recommendations. Will implement them all for the next I-build.
Reopening...
Implemented all recommendations made in comment #5. Code will be in the 0511-0800 I-build.
