Prevent CME in LaunchBarManager

[trancexpress]

This change synchronizes access to LaunchBarManager.descriptors, to avoid a ConcurrentModificationException when adding descriptors for 2 launches at the same time.

Fixes: #262

[trancexpress]

I'm not sure this is the best change, it adds a lot of code and there are other collections in LaunchBarManager that might likewise be accessed in parallel (and so cause problems). Also its not clear what the consequences of the race condition (that results in the CME) are...

Maybe a better option is to make launchConfigurationAdded(), launchConfigurationRemoved() and launchConfigurationChanged() in LaunchBarManager synchronized... But that will need (a lot) more examination of the code or someone knowledgeable of the code to review the change. Since it might introduce deadlocks, in case LaunchBarManager itself calls code that also holds who-knows-what locks.

[github-actions]

Test Results

     593 files       593 suites   16m 34s ⏱️
10 136 tests 10 113 ✔️ 22 💤 1 ❌
10 152 runs  10 129 ✔️ 22 💤 1 ❌

For more details on these failures, see this check.

Results for commit 487ef92.

♻️ This comment has been updated with latest results.

[jonahgraham]

Failed test is unrelated, see #259

[jonahgraham]

Can we simply change the maps to ConcurrentHashMap or wrap it in Collections.synchronizedMap (for the linked one)? That would mean that future editors of the code won't need to worry about getting it right as it would "just work" (moreso for concurrent hash map, less so for wrapper synchronized map)? That would address your other issues. I don't think there is a performance issue in the code of having concurrent hash maps.

I am happy with this change as is, but I'll hold off merging until you have a chance to consider my question.

[jonahgraham]

I am pretty sure you don't need to save off the isContained here, I think throwing from within the synchronized block is fine:

syncrhonized(descriptors) {
    if (descriptor != null && !descriptors.containsValue(descriptor)) {
        throw new IllegalStateException(Messages.LaunchBarManager_1);
    }
}

If you need to optimize to not get the lock if descriptor == null then a partial split is fine too:

if (descriptor != null) {
    syncrhonized(descriptors) {
        if (!descriptors.containsValue(descriptor)) {
            throw new IllegalStateException(Messages.LaunchBarManager_1);
        }
    }
}

[jonahgraham]

I'm going to approve these changes as they are now. Fixing up the code more widely can be a discussion for another day, but this change is a move in the right direction.

[trancexpress]

Can we simply change the maps to ConcurrentHashMap or wrap it in Collections.synchronizedMap (for the linked one)? That would mean that future editors of the code won't need to worry about getting it right as it would "just work" (moreso for concurrent hash map, less so for wrapper synchronized map)? That would address your other issues. I don't think there is a performance issue in the code of having concurrent hash maps.

I am happy with this change as is, but I'll hold off merging until you have a chance to consider my question.

Sorry I must have missed the e-mail notification for this comment.

I actually thought about making the container a synchronized one, but there are operations in LaunchBarManager which are odd to me. From what I can tell, in some places the code takes care to ensure the order in the LinkedHashMap is the correct one (i.e. the order the code needs) and this is done by first removing an element and then putting the new one... such sequences have to either still be in a synchronized block, or they have to be carefully replaced by single call APIs from the map. Since I'm not sure if the "replacement" methods of the map keep the order in the LinkedHashMap as expected (and even if they do, whether this is behavior as specified by documentation, or just an accident of implementation).

What I can do of course is keep the operations as they are, but wrap only the more complicated sequences with synchronized blocks. That will already reduce the amount of added code for this change.

My bigger concern was that synchronizing only on the one map here might not be solving race condition problems, just hiding the CMEs. As I've commented at the start of the PR. But since the change got merged I'll look into cleaning the code a bit. I guess better to not have the CMEs, even if not all problems are solved.

[jonahgraham]

Your explanation is good, thanks for taking the time to document it out.

I guess better to not have the CMEs, even if not all problems are solved.

I agree.