[MGNLSLOCK-4] Modifications to map of concurrently editing users should be synchronized Created: 01/Dec/10  Updated: 09/Sep/11  Resolved: 07/Sep/11

Status: Closed
Project: Magnolia Soft Locking Module
Component/s: None
Affects Version/s: 1.0
Fix Version/s: 1.0.2

Type: Bug Priority: Major
Reporter: Tobias Mattsson Assignee: Federico Grilli
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
relation
is related to MGNLSLOCK-11 Locks are not released Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

Even though the map is a ConcurrentMap, DefaultSoftLockingSupport first probes the value, then checks if its null to create a new map to put in. This is a race condition that can result in lost updates.

A lock for modification should be introduced and used in addCurrentUserToEditingList(). The lock should also be held in removeCurrentUserFromEditingList() allowing for it to probe if the list becomes empty and if it is remove it from the map.



 Comments   
Comment by Federico Grilli [ 10/Jan/11 ]

I'll postpone this for a later release both because, all in all, it's not a critical issue (we might lose some updates in the worst case) and because this seems to be trickier than it might look at a first glance. The problem is that ConcurrentHashMap and synchronized "don't get along well" and you are supposed to use instead that class's atomic methods such as putIfAbsent and replace. But what we need here requires a slightly more complex condition checking than what is carried out by those methods which only test if a map with a certain key or value exists before putting or replacing it. I guess, we are in a situation similar to that described here http://stackoverflow.com/questions/1395010/concurrenthashmap-conditional-replace. At this point we might want to fall back to a plain HashMap and use synchronized where appropriate. Or maybe I did not quite understand ConcurrentHashMap and our use case and I am overcomplicating things. In this case, a clarifying help would be appreciated

Comment by Tobias Mattsson [ 10/Jan/11 ]

I think that adding a

private final Object modificationMonitor = new Object();

and then synchronizing on it while modifying the map should be enough. That would make modifications sequential while still allowing reads to be concurrent. Modifications and reads on the map are still concurrent, but there's never more than one thread modifying it at once. Hence its safe for the modifying thread to query the map and then change it based on the result.

Comment by Jan Haderka [ 07/Sep/11 ]

Why is test set up in <init> and not in setUp() method? Specially if we still unlock it in tearDown(). One time setup only breaks isolation of the tests.

Generated at Mon Feb 12 07:14:22 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.