This class uses AtomicBooleans. Is it thread safe ?

Question

I don't like to lock up my code with synchronized(this), so I'm experimenting with using AtomicBooleans. In the code snippet, XMPPConnectionIF.connect() makes a socket connection to a remote server. Note that the variable *connecting is only ever used in the connect() method; whereas *connected is used in every other methods that needs to use the *xmppConn. My questions are listed after the code snippet below.

private final AtomicBoolean _connecting = new AtomicBoolean( false );
private final AtomicBoolean _connected = new AtomicBoolean( false ); 
private final AtomicBoolean _shuttingDown = new AtomicBoolean( false ); 
private XMPPConnection _xmppConn;
/**
 * @throws XMPPFault if failed to connect
 */
public void connect() 
{
    // 1) you can only connect once
    if( _connected.get() )
        return;

    // 2) if we're in the middle of completing a connection, 
    //    you're out of luck
    if( _connecting.compareAndSet( false, true ) )
    {
        XMPPConnectionIF aXmppConnection = _xmppConnProvider.get();
        boolean encounteredFault = false;

        try
        {
            aXmppConnection.connect(); // may throw XMPPException
            aXmppConnection.login( "user", "password" ); // may throw XMPPException
            _connected.compareAndSet( false, true );
            _xmppConn = aXmppConnection;
        }
        catch( XMPPException xmppe )
        {
            encounteredFault = true;
            throw new XMPPFault( "failed due to", xmppe );
        }
        finally
        {
            if( encounteredFault )
            {
                _connected.set( false );
                _connecting.set( false );
            }
            else
                _connecting.compareAndSet( true, false );
        }
    }
}

1. Based on my code, is it thread safe to the point that if 2 threads attempt to call connect() at the same time, only one connection attempt is allowed.

2. In the finally block, I am executing two AtomicBoolean.set(..) in succession, will be there be a problem, since during the gap between these 2 atomic calls, some threads might call *connected.get() in other methods ?

3. When using *xmppConn, should I do a *synchronized( xmppConn ) ?

UPDATE Added missing login call into the method.

Answer 1

Keep in mind that using 3 AtomicBooleans is not the same as guarding those three variables with a single lock. It seems to me like the state of those variables constitutes a single state of the object and thus that they should be guarded by the same lock. In your code using atomic variables, it's possible for different threads to update the state of _connected, _connecting, and _shuttingDown independently -- using atomic variables will only ensure that access to the same variable is synchronized between multiple threads.

That said, I don't think synchronizing on the this is what you want to do. You only want to synchronize access to the connection state. What you could do is create an object to use as the lock for this state without getting the monitor on this. Viz:

class Thing {
  Boolean connected;
  Boolean connecting;
  Boolean shuttingDown;
  Object connectionStateLock = new Object();

  void connect() {
    synchronized (connectionStateLock) {
      // do something with the connection state.
    }
  }

  void someOtherMethodThatLeavesConnectionStateAlone() {
    // free range thing-doing, without getting a lock on anything.
  }
}

If you're doing concurrent programming in Java, I would highly recommend reading Java Concurrency In Practice.

Answer 2

1. Yes. The variable _connecting acts as a test-and-set lock that prevents multiple concurrent connection attempts.

2. No problem -- even if another thread reads _connected between the writes, _connecting will prevent it from attempting to connect concurrently.

3. Yes, assuming that its methods are not already thread-safe.

That being said, your connect() method would drive me nuts in its current form, since it doesn't necessarily connect or throw an exception. You could add a spin loop, but that's not really a good fit because for all but the shortest of network hops from a multiprocessor machine, it will be more efficient to yield. The low-level concurrency primitives, moreover, are a lot more error-prone than synchronized -- I strongly recommend you stick with synchronized.

Answer 3

1. Yes its definitely satisfied. as _connecting.compareAndSet( false, true ) will allow only one thread to get in.

2. You dont need to set _connected.set( false ); as it is never set to true if exception happened. Yes its possible not due to succession but till you have not set connecting to false other Threads trying to connect wont be doing thinking that a connection is in progress.

3. Yes if xmppConn is not Thread Safe.

Answer 4

I doubt your program will be thread-safe. I am no Java Memory Model guru, but from what I have learned operations can be arranged and results of operations may not be visible to other threads in the order you expect.

Consider if for example setting _connected to true is executed before the connect() method is fully executed? Another thread could think that you are connected even though you aren't. This is just conjecture - I am not sure that particular problem can occur at all.

My point is rather that the sort of locking you are trying to do is extremely tricky to get right. Stick with synchronized or use the locks in the java.util.concurrent.locks package.

Answer 5

I think others are covering correctness adequately in their comments. My only additional comment would be that I am concerned a bit about the placement of the release in the finally. Seems like you might really want to wrap the whole block there (including the _xmppConnProvider.get() call) in a try { } finally { } that would guarantee you would always release the lock. Otherwise, some kind of unchecked exception could happen in there and leave you in an unrecoverable state.

Stylistically, I regard this code as much more difficult to reason about than simply using synchronized/Lock to achieve mutual exclusion. I would start with code that is easy to reason about and only make it more complicated if you can prove that this is a hot spot.