search

imglib / imglib2-roi

Regions of interest (ROIs) and labelings for ImgLib2
Other
8 stars 8 forks source link

Synchronizing update() of LabelRegions #19

Closed dietzc closed 8 years ago

dietzc commented 8 years ago

https://github.com/imglib/imglib2-roi/blob/master/src/main/java/net/imglib2/roi/labeling/LabelRegions.java#L333

Shouldn't the generation check be synchronized, too? If I have to processes running in parallel both will update the list, even if generations match after the first thread has updated all lists.

tpietzsch commented 8 years ago

@dietzc Probably you are right. I think also something has to be done about https://github.com/imglib/imglib2-roi/blob/master/src/main/java/net/imglib2/roi/labeling/LabelRegion.java#L147 I'll need to look at it in more detail.

dietzc commented 8 years ago

The unsynchronized update() causes also concurrent modification exceptions (if thread one updates while thread two accesses the existing labels).

tpietzsch commented 8 years ago

can you give a more detailed scenario and stacktrace of exception?

dietzc commented 8 years ago

I will try to write a reproducible test.

dietzc commented 8 years ago

see: https://gist.github.com/dietzc/fa6eac96791b67c2fed4 it's not too easy to reproduce outside KNIME. However, the problem becomes visible if you run this test and set a breakpoint in line https://gist.github.com/dietzc/fa6eac96791b67c2fed4#file-gistfile1-txt-L54 and then step into getExistingLabels() (one thread after the other) and run them as soon as they passed the generation check in the update(), then you will experience a ConcurrentModificationException.

Important: Start both threads at the same time. Exception you should get:

java.util.concurrent.ExecutionException: java.util.ConcurrentModificationException
    at java.util.concurrent.FutureTask.report(FutureTask.java:122)
    at java.util.concurrent.FutureTask.get(FutureTask.java:192)
    at net.imglib2.roi.labelregions.LabelRegionsTest.concurrencyTest(LabelRegionsTest.java:74)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
    at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:459)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:675)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:382)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:192)
Caused by: java.util.ConcurrentModificationException
    at java.util.HashMap$HashIterator.nextNode(HashMap.java:1429)
    at java.util.HashMap$KeyIterator.next(HashMap.java:1453)
    at net.imglib2.roi.labelregions.LabelRegionsTest$1.call(LabelRegionsTest.java:56)
    at net.imglib2.roi.labelregions.LabelRegionsTest$1.call(LabelRegionsTest.java:1)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
dietzc commented 8 years ago

More problems related to getExistingLabels(): If we don't synchronize the generation check, potentially Thread A may get an empty existing labels list (as generation is the same, but list has not been updated by the Thread which is updating at the moment).

dietzc commented 8 years ago

fixed with https://github.com/imglib/imglib2-roi/commit/f31dc3964f628ed704f33e9201696df76a55b1a2