On 7/28/07, Adam Fisk <[EMAIL PROTECTED]> wrote: > > I actually don't think the problem you're specifically talking about is an > issue. The box you mentioned in Section 3.5.3 of "Java Concurrency in > Practice" lists the options for achieving safe publication. One option > mentions using volatile, but the last one reads "Storing a reference to it > into a field that is properly guarded by a lock." In this case, as far as > I > can tell, the "this.selector" variable is properly guarded by a > lock. That > is, each time it's modified, it's protected with a lock in the form of a > synchronized block. Note, though that "properly guarded by a lock" > simply > means there's a lock on it every time it's *written*, not every time it's > read. So there doesn't need to by synchronization on each read.
That is mistake that many people make, you need synchronization for both writing and reading. When thread-A is writing some values while holding a lock, the written values are guaranteed to be seen by any other thread that acquires that same lock after thread-A released it, but there is no guarantee that threads that did not acquire that lock will see those writes. So I guess, that James is right, and this should be fixed. quote from http://www.ibm.com/developerworks/java/library/j-threads1.html "To make your programs thread-safe, you must first identify what data will be shared across threads. If you are writing data that may be read later by another thread, or reading data that may have been written by another thread, then that data is shared, and you must synchronize when accessing it. Some programmers are surprised to learn that these rules also apply in situations where you are simply checking if a shared reference is non-null. Many people find these definitions surprisingly stringent. It is a commonly held belief that you do not need to acquire a lock to simply read an object's fields, especially since the JLS guarantees that 32-bit reads will be atomic. Unfortunately, this intuition is incorrect. Unless the fields in question are declared volatile, the JMM does not require the underlying platform to provide cache coherency or sequential consistency across processors, so it is possible, on some platforms, to read stale data in the absence of synchronization. " Article was written by Brian Goetz, one of the authors of the excellent Java Concurrency in Practice book. Maarten At least that's my take. That said, the whole opening and closing of the > selector (why the selector can ever be null), is still fishy to me. Can > anyone shed light on that? Does it have to do with performance gains of > closing idle selectors? > > Thanks. > > -Adam > > Oh, here's the full text from the gray box James mentioned if anyone else > wants to give it a look: > > --------------------------- > To publish an object safely, both the reference to the object and the > object's state must be made visible to other threads at the same time. A > properly constructed object can be safely published by: > > - > > Initializing an object reference from a static initializer; > - > > Storing a reference to it into a volatile field or AtomicReference; > - > > Storing a reference to it into a final field of a properly constructed > object; or > - > > Storing a reference to it into a field that is properly guarded by a > lock. > > ---------------------------- > > > On 7/28/07, James Im <[EMAIL PROTECTED]> wrote: > > > > In org.apache.mina.transport.socket.nio.SocketIoProcessor I have a bad > > _feeling_ about the concurrency of these 2 methods: > > > > void flush(SocketSessionImpl session) { > > scheduleFlush(session); > > Selector selector = this.selector; > > if (selector != null) { > > selector.wakeup(); > > } > > } > > void updateTrafficMask(SocketSessionImpl session) { > > scheduleTrafficControl(session); > > Selector selector = this.selector; > > if (selector != null) { > > selector.wakeup(); > > } > > } > > > > > > I have a problem specifically with this piece of code: > > > > Selector selector = this.selector; > > if (selector != null) { > > selector.wakeup(); > > } > > > > The code access 'this.selector' in an unsynchronized manner from other > > threads and I don't know if some visibility/safe publication issues > > might arise here. > > > > The problem is not that you call "selector.wakeup();" from other > > threads, the problem is rather the fact that you are accessing the > > variable 'selector' without synchronization. > > > > In the same class when you change the variable 'selector' you > > synchronize around the 'lock' object to take care of synchronization > > issues. For me that seems to indicate that on one hand you are taking > > care of visibility/safe publication issues and on the other one it is > > not taken care of. > > > > If you happen to have the book "Java Concurrency in Practice", please > > read again "3.5 Safe publication" on page 49-54. It seems to me that > > the gray rectangle (in 3.5.3) hints that to avoid the problem we might > > need to store the reference to the selector into a volatile field. > > > > private volatile Selector selector; > > > > What do you think? From a visibility/safe publication stand point, do > > you see a problem here? > > > > _________________________________________________________________ > > Ta' på udsalg året rundt på MSN Shopping: http://shopping.msn.dk - her > > finder du altid de bedste priser > > > > >
