mina-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam Fisk"...@lastbamboo.org>
Subject Re: Unsafe concurrency issue in SocketIoProcessor ?
Date Sat, 28 Jul 2007 18:44:03 GMT
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.

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 <im-james@hotmail.com> 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
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message