-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ConcurrentModificationException when working with measurements #1444
Comments
Creating a new issue because from the stack trace it looks like a different issue (related to measurement lists, not object descendants) so any fix should reference this instead. |
That would be useful because I was not able to replicate the issue (on MacOS, I will try on Linux later). However, based on the exception, I think the problem is that the names ArrayList of the AbstractNumericMeasurementList class is accessed from multiple threads without synchronization. Using a |
I think the list shouldn't be directly accessed elsewhere, and the |
Ah, interesting... yes that looks like a horribly subtle concurrency bug. Synchronizing It does seem that the only time I tried to replicate the issue with def pathObject = PathObjects.createDetectionObject(ROIs.createEmptyROI())
java.util.stream.IntStream.range(1, 1000)
.parallel()
.each(i -> {
def ml = pathObject.getMeasurementList()
ml.put("Hello " + i, Math.random())
ml.close()
}) ...but I failed to get it to throw any exception. |
I think all access to Switching to |
So I confirm that all access to any mutable variable should be synchronized. From "Concurrency in Practice":
I can refactor |
I think that I hadn't really appreciated that using the list as key in the map would result in The purpose was to ensure that identical string lists aren't duplicated for all objects - since there can be millions, which could easily cost hundreds of MB overhead. Basically, we'd like measurements to be accessible like The current design is probably very suboptimal, but it's quite core to QuPath (for performance, memory use and serialization) so any major change would need to be very carefully checked. |
Great! How?
Looks good, is there a way to check if it has any significant performance impact? I'll check this out as well when I can (I expect it's fine, I've just had synchronization surprise me in the past). |
The exception occurs each time I run
It has, because running |
Wow, thanks, that replicates the issue for me too. This sounds like a bug / intuitive behavior within the Delaunay triangulation. It's concerning that measurements can be added multiple times to the same objects. It suggests that the results might not be fully deterministic, depending upon the status of the object hierarchy and precisely which annotations are selected. I'm reluctant to fix the underlying issue in a 0.0.x release, but we should try to replace the command entirely. An implementation with As I understand it, this shows that the existing Delaunay command should not be used for nested annotations that contain detections. Single annotations, or annotations arranged in a 'flat' way (so that the same detection is not a descendent of more than one selected annotation) should be ok. |
OK, so should I discard the PR? If this is the only issue occuring with |
Oh no, please keep the PR for now! I'll check it out in more detail soon - but you've demonstrated that there is a concurrency bug with the measurement list. It just wouldn't have arisen if the Delaunay command wasn't buggy too. Similarly, the performance probably wouldn't have changed noticeably if the Delaunay command wasn't problematic... so this may not be a major issue in other contexts. One thing to check would be 'Add smoothed measurements' with lots of cells, since this should add a lot of measurements in parallel - but I think only one thread should be accessing each measurement list. Therefore I hope synchronization doesn't cause substantial overhead. In any case, I think |
Reopening this as it's very similar and still happening in QuPath 0.5.0
A user had an annotation inside which there was another annotation filled with detections (over 5000)
When runing "Delaunay cluster features 2D" we ran into
I can ask them to share a QuPath project if it's useful to you.
The issue appeared on a built from source linux QuPath if that's of any use.
All the best and happy new year
Originally posted by @lacan in #1182 (comment)
The text was updated successfully, but these errors were encountered: