You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by James Im <im...@hotmail.com> on 2007/07/28 19:30:53 UTC
Unsafe concurrency issue in SocketIoProcessor ?
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
Re: Unsafe concurrency issue in SocketIoProcessor ?
Posted by Trustin Lee <tr...@gmail.com>.
On 7/29/07, James Im <im...@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?
Right, there's a visibility problem, but it won't hurt MINA because
Selector is called with timeout (1 second). At worst case, wakeup
will be delayed for 1 second. And the callers of wakeup() calls
wakeup() mostly after some synchronized block (e.g.
IoSession.write()), so the possibility that something goes wrong is
very close to zero.
Trustin
--
what we call human nature is actually human habit
--
http://gleamynode.net/
--
PGP Key ID: 0x0255ECA6
Re: Unsafe concurrency issue in SocketIoProcessor ?
Posted by Adam Fisk <a...@lastbamboo.org>.
I stand corrected! Good stuff. Simply making selector volatile might be
the way to go, then, as James suggested, simply because it would eliminate
the more error-prone need to wrap every access in a synchronized block.
This totally makes sense after thinking about it. Each thread still has its
own copy of the variable, and it only gets the updated value when it
synchronizes on the same lock that updated it.
There's a very similar quote from Java Concurrency in Practice. Here's the
whole of section 2.4. I'm sure the publisher won't mind a little bending of
the copyright rules in this case, as we're if anything advertising their
book:
2.4. Guarding State with Locks
Because locks enable serialized [8] <javascript:moveTo('ch02fn08');> access
to the code paths they guard, we can use them to construct protocols for
guaranteeing exclusive access to shared state. Following these protocols
consistently can ensure state consistency.
[8] Serializing access to an object has nothing to do with object
serialization (turning an object into a byte stream); serializing access
means that threads take turns accessing the object exclusively, rather than
doing so concurrently.
Compound actions on shared state, such as incrementing a hit counter
(read-modify-write) or lazy initialization (check-then-act), must be made
atomic to avoid race conditions. Holding a lock for the entire duration of a
compound action can make that compound action atomic. However, just wrapping
the compound action with a synchronized block is not sufficient; if
synchronization is used to coordinate access to a variable, it is
needed everywhere
that variable is accessed. Further, when using locks to coordinate access to
a variable, the same lock must be used wherever that variable is accessed.
It is a common mistake to assume that synchronization needs to be used only
when writing to shared variables; this is simply not true. (The reasons for
this will become clearer in Section
3.1<http://safari.oreilly.com/0321349601/ch03lev1sec1#ch03lev1sec1>
.)
For each mutable state variable that may be accessed by more than one
thread, all accesses to that variable must be performed with the same lock
held. In this case, we say that the variable is guarded by that lock.
In SynchronizedFactorizer in Listing
2.6<http://safari.oreilly.com/0321349601/ch02lev1sec3#ch02list06>,
lastNumber and lastFactors are guarded by the servlet object's intrinsic
lock; this is documented by the @GuardedBy annotation.
There is no inherent relationship between an object's intrinsic lock and its
state; an object's fields need not be guarded by its intrinsic lock, though
this is a perfectly valid locking convention that is used by many classes.
Acquiring the lock associated with an object does not prevent other threads
from accessing that object—the only thing that acquiring a lock prevents any
other thread from doing is acquiring that same lock. The fact that every
object has a built-in lock is just a convenience so that you needn't
explicitly create lock objects. [9] <javascript:moveTo('ch02fn09');> It is
up to you to construct locking protocols or synchronization policies that
let you access shared state safely, and to use them consistently throughout
your program.
[9] In retrospect, this design decision was probably a bad one: not only can
it be confusing, but it forces JVM implementors to make tradeoffs between
object size and locking performance.
Every shared, mutable variable should be guarded by exactly one lock. Make
it clear to maintainers which lock that is.
A common locking convention is to encapsulate all mutable state within an
object and to protect it from concurrent access by synchronizing any code
path that accesses mutable state using the object's intrinsic lock. This
pattern is used by many thread-safe classes, such as Vector and other
synchronized collection classes. In such cases, all the variables in an
object's state are guarded by the object's intrinsic lock. However, there is
nothing special about this pattern, and neither the compiler nor the runtime
enforces this (or any other) pattern of locking.
[10]<javascript:moveTo('ch02fn10');>It is also easy to subvert this
locking protocol accidentally by adding a
new method or code path and forgetting to use synchronization.
[10] Code auditing tools like FindBugs can identify when a variable is
frequently but not always accessed with a lock held, which may indicate a
bug.
Not all data needs to be guarded by locks—only mutable data that will be
accessed from multiple threads. In Chapter
1<http://safari.oreilly.com/0321349601/ch01#ch01>,
we described how adding a simple asynchronous event such as a TimerTask can
create thread safety requirements that ripple throughout your program,
especially if your program state is poorly encapsulated. Consider a
single-threaded program that processes a large amount of data.
Single-threaded programs require no synchronization, because no data is
shared across threads. Now imagine you want to add a feature to create
periodic snapshots of its progress, so that it does not have to start again
from the beginning if it crashes or must be stopped. You might choose to do
this with a TimerTask that goes off every ten minutes, saving the program
state to a file.
Since the TimerTask will be called from another thread (one managed by Timer),
any data involved in the snapshot is now accessed by two threads: the main
program thread and the Timer thread. This means that not only must the
TimerTask code use synchronization when accessing the program state, but so
must any code path in the rest of the program that touches that same data.
What used to require no synchronization now requires synchronization
throughout the program.
When a variable is guarded by a lock—meaning that every access to that
variable is performed with that lock held—you've ensured that only one
thread at a time can access that variable. When a class has invariants that
involve more than one state variable, there is an additional requirement:
each variable participating in the invariant must be guarded by the
samelock. This allows you to access or update them in a single atomic
operation,
preserving the invariant. SynchronizedFactorizer demonstrates this rule:
both the cached number and the cached factors are guarded by the servlet
object's intrinsic lock.
For every invariant that involves more than one variable, all the variables
involved in that invariant must be guarded by the same lock.
If synchronization is the cure for race conditions, why not just declare
every method synchronized? It turns out that such indiscriminate application
of synchronized might be either too much or too little synchronization.
Merely synchronizing every method, as Vector does, is not enough to render
compound actions on a Vector atomic:
if (!vector.contains(element))
vector.add(element);
This attempt at a put-if-absent operation has a race condition, even though
both contains and add are atomic. While synchronized methods can make
individual operations atomic, additional locking is requiredwhen multiple
operations are combined into a compound action. (See Section
4.4<http://safari.oreilly.com/0321349601/ch04lev1sec4#ch04lev1sec4>for
some techniques for safely adding additional atomic operations to
thread-safe objects.) At the same time, synchronizing every method can lead
to liveness or performance problems, as we saw in SynchronizedFactorizer.
On 7/28/07, Maarten Bosteels <mb...@gmail.com> wrote:
>
> On 7/28/07, Adam Fisk <a...@lastbamboo.org> 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 <im...@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
> > >
> > >
> >
>
Re: Unsafe concurrency issue in SocketIoProcessor ?
Posted by Maarten Bosteels <mb...@gmail.com>.
On 7/28/07, Adam Fisk <a...@lastbamboo.org> 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 <im...@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
> >
> >
>
Re: Unsafe concurrency issue in SocketIoProcessor ?
Posted by Adam Fisk <a...@lastbamboo.org>.
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...@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
>
>