Concurrency issues

[pbvahlst]

Hi there, nice little API :-)

It has some concurrency issues though which needs attention, I will be happy to give some pointers if the project is still active. E.g.

OSCPacketDispatcher: You need to use a ConcurrentHashMap for this to be thread safe, if you where to add a listener after OSCPortIn has started receiving messages there could be visibility issues of the newly added listener and ConcurrentModificationExceptions could occur

OSCMessage: is not Immutable and therefore not thread safe. This can cause issues with visibility when crossing thread boundaries

OSCPortIn: The listening flag will be accessed by multiple threads and is not currently thread safe. Should be changed to volatile or accessed In thread safe manner. If not this could cause visibility issues and in worst case it will never stop running because it want see the state change of the listening flag.

.... More issues....

[ciyer]

Hi,

Thanks for taking such a close look at the code.

2013/9/21 Peter Vahlstrup [email protected]

OSCPacketDispatcher:

You need to use a ConcurrentHashMap for this to be thread safe, if you where to add a listener after OSCPortIn has started receiving messages there could be visibility issues of the newly added listener and ConcurrentModificationExceptions could occur

The OSCPortIn is not designed to allow modifying the address space while running.

Switching to a ConcurrentHashMap would prevent the possibility of seeing an incoherent state of the underlying map in this situation, but it would not make the class thread safe. The thread safety would depend on how it was used (e.g., think about the case that a client wants to add a new listener for an address if there was none already registered -- additional synchronization would be required).

We should update the class documentation to clarify the intended use of the class.

OSCMessage: is not Immutable and therefore not thread safe. This can cause issues with visibility when crossing thread boundaries

OSCMessage is not designed to be mutated in multiple threads. This is unfortunately not reflected in the design -- if I were to design the API again, I would use the builder pattern to construct message objects and have all fields of the message objects final...

We should document make the intended use of the class clear in the class documentation.

OSCPortIn: The listening flag will be accessed by multiple threads and is not currently thread safe. Should be changed to volatile or accessed In thread safe manner. If not this could cause visibility issues and in worst case it will never stop running because it want see the state change of the listening flag.

This is indeed a bug. Nice catch!

Sekhar

C. Ramakrishnan [email protected]

[pbvahlst]

Happy to help :)

Switching to a ConcurrentHashMap would prevent the possibility of seeing an incoherent state of the underlying map in this situation, but it would not make the class thread safe. The thread safety would depend on how it was used (e.g., think about the case that a client wants to add a new listener for an address if there was none already registered -- additional synchronization would be required).

Agreed, sorry for vague formulation

OSCMessage is not designed to be mutated in multiple threads. This is unfortunately not reflected in the design -- if I were to design the API again, I would use the builder pattern to construct message objects and have all fields of the message objects final...

Even though it shouldn't be mutated in another thread there will (could) still be visibility issues if the setters and getters are not synchronized or the fields are not final. That is if the object crossed thread boundaries (on multi-core processors). the Java Memory Model describes this: http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5-110

[Burtan]

I think there is one more threading problem. When you send data one OSCSerializer is built per sending operation, as it is not thread safe, but they all share the buffer from the OSCPortOut.

[DiMoSe]

I'm consistently getting an error when receiving values quickly. I figure it's not hard to avoid, I really don't need to receive the instructions that quickly but thought it related to this thread. If you need any more info I can provide to help fix this let me know, it's quite easy to reproduce.

Exception in thread "Thread-1" java.util.ConcurrentModificationException
	at java.base/java.util.ArrayList$Itr.checkForComodification(ArrayList.java:1013)
	at java.base/java.util.ArrayList$Itr.next(ArrayList.java:967)
	at com.illposed.osc.OSCPacketDispatcher.dispatchMessageNow(OSCPacketDispatcher.java:401)
	at com.illposed.osc.OSCPacketDispatcher.dispatchPacket(OSCPacketDispatcher.java:287)
	at com.illposed.osc.OSCPacketDispatcher.handlePacket(OSCPacketDispatcher.java:300)
	at com.illposed.osc.transport.OSCPortIn.run(OSCPortIn.java:199)
	at java.base/java.lang.Thread.run(Thread.java:831)