CODEWITHSUNDEEP
java
arge
arge

Reputation: 635

Method synchronizes on an updated field

My java code looks like this:

class xHandler
{
    private Channel _channel;

    // Methods...

    void init()
    {
        _channel = new Channel(...);

        synchronized (_channel)
        {
            // Do some stuff here...e.g.
            _channel.send("...");
        }
    }
}

In other files(threads) I create instances of Channel and send stuff with the use of the object, but only in the init method mentioned above I need it to be sync'ed and no other thread should open a channel and send something during this period.

FindBugs gives me a warning called: Method synchronizes on an updated field

This method synchronizes on an object referenced from a mutable field. This is unlikely to have useful semantics, since different threads may be synchronizing on different objects.

How can I get this issue right? Is it possible to trigger this issue easily with a simple test?

Upvotes: 2

Views: 431

Answers (3)

Anthony Accioly
Anthony Accioly

Reputation: 22461

_channel = new Channel(...);
synchronized (_channel)

I need it to be sync'ed and no other thread should open a channel and send something during this period.

Seems like your code is not doing what you are thinking that it is doing. Since you are creating a new Channel object per xHandler instance, and acquiring this specific Channel lock, two threads may run concurrently.

  1. xHandler instance 1 creates Channel instance 1 and acquires it's lock
  2. xHandler instance 2 creates Channel instance 2 and acquires it's lock
  3. Both run concurrently

What you really need to do is either:

  1. Share the specific Channel object that you want to sync for multiple xHandler instances (Example: receive a instance of Channel on xHandler constructor and assign it to _channel, remove the _channel = new Channel(...); line and keep your sync code as it is).
  2. Sync on Channel.class which will mean that two different instances of xHandler will never be able to use any Channel simultaneously.

Example of sharing a specific Channel for two different xHandlers:

class xHandler
{
    private final Channel _channel;

    public xHandler(Channel channel)
    {
        this._channel = channel  
    }

    // Methods...

    void init()
    {           
        synchronized (_channel)
        {
            // Do some stuff here...e.g.
            _channel.send("...");
        }
    }

            public static void main(String[] args) 
            {
                Channel myChannel = new Channel(...);
                xHandler xHand1 = new xHandler(myChannel);
                xHandler xHand2 = new xHandler(myChannel);
                // Code to create / start your threads. 
                // xHand1 and xHand2 will not use myChannel simultaneously  
            }

}

Upvotes: 1

Adrian
Adrian

Reputation: 5681

You keep on creating a new lock every time the method is run. Thus you are negating the effects of locking. The lock will always allow new threads in because there will be no threads waiting on the newly created object (there could be every now and then through some race condition). Each thread basically has its own lock (instead of a shared lock).

Move channel outside of the method.

Upvotes: 2

amit
amit

Reputation: 178411

Note that synchronized is acquiring lock on an object and not on a variable!

Each invokation of this method will have a lock on a different object, so the synchronized block in here is actually redundant, since you modify it when you enter init(). [so as the warning says, different threads are being synchronized on different objects].

What will happen if two threads will try to invoke init() concurrently? Both of them might enter the critical section at the same time, and this is what you are warned about.

To solve it, you might want to declare _cahnnel as final, and initialize it in the constructor, and not init().

Upvotes: 3

Related Questions