Adding in concurrent logic to help prevent ConcurrentModificationException #89
Conversation
…ption, also appears to have a very minor boon to performance.
There was a problem hiding this comment.
It's cool you investigated a new locking mechanism, but I don't understand how you view its behaviour differently than the existing synchronized (..) blocks. Some of the work here seems superfluous, and some of your unlocks aren't in finally clauses.
Can you take a pass at my questions and help me understand? Thanks!
| // so that new IPC vehicle data will be created and cached and | ||
| // made available to the API. | ||
| VehicleDataCache.getInstance().updateVehicle(vehicleState); | ||
| l.unlock(); |
There was a problem hiding this comment.
again, if the updateVehicle(vehicleState) is at issue by being outside of the synchronized block, then consider moving it.
| @@ -217,6 +220,7 @@ public Indices increment(long epochTime) { | |||
| } | |||
| } | |||
| } | |||
| l.unlock(); | |||
There was a problem hiding this comment.
Can you explain why a lock is needed here?
| @@ -88,16 +90,19 @@ private static String threadNameWithCounter(String name) { | |||
| count++; | |||
| } | |||
| threadNameCountMap.put(name, count); | |||
| l.unlock(); | |||
There was a problem hiding this comment.
If the threadNameCountMap.put is causing the issue, consider moving it inside the synchronized block.
| @@ -141,6 +146,7 @@ public void run() { | |||
| System.exit(-1); | |||
| } | |||
| } finally { | |||
| l.unlock(); | |||
There was a problem hiding this comment.
ok, you really need to convince me this is necessary. Locking an entire thread seems completely unreasonable. I suspect you don't understand how this thread is used.
There was a problem hiding this comment.
I can remove this and retest.
| @@ -165,7 +167,7 @@ public void run() { | |||
| // filter | |||
| // the next one | |||
| avlReports.put(avlReport.getVehicleId(), avlReport); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
it the avlReports.put(...) is the problem here, I suggest extending the synchronized (avlReports) block instead. The Reentrant lock doesn't give you anything different as I understand it.
Adding in concurrent logic to help prevent ConcurrentModificationException, also appears to have a very minor boon to performance.