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 20:38:07 GMT
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 <mbosteels.dns@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-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