Details
-
Bug
-
Status: Resolved
-
Resolution: Done
-
Boron
-
None
-
None
-
Operating System: All
Platform: All
-
6147
Description
In ElanPacketInHandler.onPacketReceived():
byte[] srcMac = res.getSourceMACAddress();
// You create a new String here
String macAddress = NWUtil.toStringMacAddress(srcMac);
// You call setupMacFlows with the new created String
ElanUtils.setupMacFlows(…., macAddress)
In ElanUtils.setupMacFlows():
synchronized (macAddress) {
...
}
The macAddress String that the synchronized block is using as a mutex was created locally. If the same mac value will be used in a different place or by a different thread, it will have its own reference, thus not locking anything.
A naive solution will be to use String.intern() but this is a very bad practice, as illustrated here: http://stackoverflow.com/questions/461896/what-is-the-most-frequent-concurrency-issue-youve-encountered-in-java/463437#463437.
My suggested solution is to hold a map between macAddress to Object() and synchronized on the Object.
Map will be created like this:
CacheBuilder.newBuilder().weakValues()
To obtain the lock, you will use:
Object lock = Cache.asMap().putIfAbsent(mac, new Object());
Because we are using weakValues(), there is no need to remove the entries from the map, as they will be garbage collected if there is no live reference to them.