Skip to main content
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorCarsten Hiesserich2015-06-04 14:48:19 -0400
committerCarsten Hiesserich2015-06-04 14:51:45 -0400
commit436938603dc178fcc587f39d348c9b557e82f015 (patch)
tree0ae099f18b651b7175da111b558b4a527f51d381
parent459fad079da3de4b70c72cd9419df9bba5605611 (diff)
downloadorg.eclipse.mylyn.docs.vex-436938603dc178fcc587f39d348c9b557e82f015.tar.gz
org.eclipse.mylyn.docs.vex-436938603dc178fcc587f39d348c9b557e82f015.tar.xz
org.eclipse.mylyn.docs.vex-436938603dc178fcc587f39d348c9b557e82f015.zip
fix random threading problem in ConfigurationRegistryImpl
The old implementation used Job.getState to check if the loader has been started. As getState is not thread safe, this caused random failures (Exception: The configurations are not loaded yet.) when running the test suite. Change-Id: I6b0c475117e52619e643ded9b5d3459dd09acae9 Signed-off-by: Carsten Hiesserich <carsten.hie@gmail.com>
-rw-r--r--org.eclipse.vex.ui.tests/src/org/eclipse/vex/ui/internal/config/tests/ConfigurationRegistryTest.java5
-rw-r--r--org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigLoaderJob.java5
-rw-r--r--org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationLoader.java2
-rw-r--r--org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationRegistryImpl.java64
4 files changed, 40 insertions, 36 deletions
diff --git a/org.eclipse.vex.ui.tests/src/org/eclipse/vex/ui/internal/config/tests/ConfigurationRegistryTest.java b/org.eclipse.vex.ui.tests/src/org/eclipse/vex/ui/internal/config/tests/ConfigurationRegistryTest.java
index 64dc8736..bf85db74 100644
--- a/org.eclipse.vex.ui.tests/src/org/eclipse/vex/ui/internal/config/tests/ConfigurationRegistryTest.java
+++ b/org.eclipse.vex.ui.tests/src/org/eclipse/vex/ui/internal/config/tests/ConfigurationRegistryTest.java
@@ -177,11 +177,6 @@ public class ConfigurationRegistryTest {
}
@Override
- public boolean isLoading() {
- return false;
- }
-
- @Override
public List<ConfigSource> getLoadedConfigSources() {
return loadedConfigSources;
}
diff --git a/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigLoaderJob.java b/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigLoaderJob.java
index 8bef8e45..e69735ca 100644
--- a/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigLoaderJob.java
+++ b/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigLoaderJob.java
@@ -109,11 +109,6 @@ public class ConfigLoaderJob extends Job implements ConfigurationLoader {
}
@Override
- public boolean isLoading() {
- return getState() == Job.RUNNING || getState() == Job.WAITING;
- }
-
- @Override
public void load(final Runnable whenDone) {
addJobChangeListener(new JobChangeAdapter() {
@Override
diff --git a/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationLoader.java b/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationLoader.java
index 2e1312b3..6c776c55 100644
--- a/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationLoader.java
+++ b/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationLoader.java
@@ -19,8 +19,6 @@ public interface ConfigurationLoader {
void load(Runnable whenDone);
- boolean isLoading();
-
List<ConfigSource> getLoadedConfigSources();
void join() throws InterruptedException;
diff --git a/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationRegistryImpl.java b/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationRegistryImpl.java
index 1195e9eb..1911e6c1 100644
--- a/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationRegistryImpl.java
+++ b/org.eclipse.vex.ui/src/org/eclipse/vex/ui/internal/config/ConfigurationRegistryImpl.java
@@ -8,6 +8,7 @@
* Contributors:
* John Krasnay - initial API and implementation
* Igor Jacy Lino Campista - Java 5 warnings fixed (bug 311325)
+ * Carsten Hiesserich - changed lock mechanism to fic random test failures
*******************************************************************************/
package org.eclipse.vex.ui.internal.config;
@@ -15,6 +16,9 @@ import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.locks.Condition;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
import org.eclipse.core.resources.IProject;
import org.eclipse.core.resources.IResourceChangeEvent;
@@ -22,8 +26,6 @@ import org.eclipse.core.resources.IResourceChangeListener;
import org.eclipse.core.resources.IResourceDelta;
import org.eclipse.core.resources.ResourcesPlugin;
import org.eclipse.core.runtime.CoreException;
-import org.eclipse.core.runtime.jobs.ILock;
-import org.eclipse.core.runtime.jobs.Job;
import org.eclipse.vex.core.internal.core.ListenerList;
import org.eclipse.vex.ui.internal.VexPlugin;
@@ -31,8 +33,10 @@ public class ConfigurationRegistryImpl implements ConfigurationRegistry {
private final ConfigurationLoader loader;
private volatile boolean loaded = false;
+ private volatile boolean loading = false;
- private final ILock lock = Job.getJobManager().newLock();
+ final Lock lock = new ReentrantLock();
+ final Condition loadedCond = lock.newCondition();
private Map<String, ConfigSource> configurationSources = new HashMap<String, ConfigSource>();
private final ListenerList<IConfigListener, ConfigEvent> configListeners = new ListenerList<IConfigListener, ConfigEvent>(IConfigListener.class);
@@ -82,32 +86,35 @@ public class ConfigurationRegistryImpl implements ConfigurationRegistry {
@Override
public void loadConfigurations() {
- lock.acquire();
+ lock.lock();
+ loading = true;
try {
loader.load(new Runnable() {
@Override
public void run() {
- lock.acquire();
+ lock.lock();
try {
configurationSources = new HashMap<String, ConfigSource>();
for (final ConfigSource configSource : loader.getLoadedConfigSources()) {
configurationSources.put(configSource.getUniqueIdentifer(), configSource);
}
loaded = true;
+ loading = false;
+ loadedCond.signalAll();
} finally {
- lock.release();
+ lock.unlock();
}
fireConfigLoaded(new ConfigEvent(ConfigurationRegistryImpl.this));
}
});
} finally {
- lock.release();
+ lock.unlock();
}
}
private List<ConfigItem> getAllConfigItems(final String extensionPointId) {
waitUntilLoaded();
- lock.acquire();
+ lock.lock();
try {
final List<ConfigItem> result = new ArrayList<ConfigItem>();
for (final ConfigSource configurationSource : configurationSources.values()) {
@@ -115,19 +122,19 @@ public class ConfigurationRegistryImpl implements ConfigurationRegistry {
}
return result;
} finally {
- lock.release();
+ lock.unlock();
}
}
private List<ConfigSource> getAllConfigSources() {
waitUntilLoaded();
- lock.acquire();
+ lock.lock();
try {
final List<ConfigSource> result = new ArrayList<ConfigSource>();
result.addAll(configurationSources.values());
return result;
} finally {
- lock.release();
+ lock.unlock();
}
}
@@ -139,41 +146,50 @@ public class ConfigurationRegistryImpl implements ConfigurationRegistry {
private void addConfigSource(final ConfigSource configSource) {
waitUntilLoaded();
- lock.acquire();
+ lock.lock();
try {
configurationSources.put(configSource.getUniqueIdentifer(), configSource);
} finally {
- lock.release();
+ lock.unlock();
}
}
private void removeConfigSource(final ConfigSource configSource) {
waitUntilLoaded();
- lock.acquire();
+ lock.lock();
try {
configurationSources.remove(configSource.getUniqueIdentifer());
} finally {
- lock.release();
+ lock.unlock();
}
}
private void waitUntilLoaded() {
- if (loaded) {
- return;
- }
- if (!loader.isLoading()) {
- throw new IllegalStateException("The configurations are not loaded yet. Call 'loadConfigurations' first.");
- }
try {
- loader.join();
+ lock.lock();
+
+ if (loading && !loaded) {
+ loadedCond.await();
+ }
+ if (loaded) {
+ return;
+ }
} catch (final InterruptedException e) {
- Thread.currentThread().interrupt();
+ } finally {
+ lock.unlock();
}
+
+ throw new IllegalStateException("The configurations are not loaded yet. Call 'loadConfigurations' first.");
}
@Override
public boolean isLoaded() {
- return loaded;
+ try {
+ lock.lock();
+ return loaded;
+ } finally {
+ lock.unlock();
+ }
}
@Override

Back to the top