Hi,

Thanks for taking such a close look at the code.

2013/9/21 Peter Vahlstrup notifications@github.com

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 cramakrishnan@acm.org

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