0

I'm trying to protect an object with a lock.

I didn't choose mutexes because of ugly syntax of "try..catch".

Browsing stackoverflow, I came to a conclusion that this is how to properly achieve my goal:

class MyClass {
    private final Object lock = new Object();
    private Channel channel = null;

    public void setChannel() {
        synchronized (lock) {
            channel = new Channel();
            synchronized (channel) {
                // setup channel
            }
        }
    }

    public void unsetChannel() {
        synchronized (lock) {
            synchronized (channel) {
                channel.close();
            }
        channel = null;
        }
    }

    public boolean isSet() {
        synchronized (lock) {
            if (channel == null)
                return false;
            synchronized (channel) {
                return channel.isActive();
            }
        }
    }
}

But it seems ugly and hard to read...

How can I improve readability of my solution?

Alexey Andronov
  • 582
  • 6
  • 28
  • why lock on a newly created object? Could also synchronise the whole method – Scary Wombat Aug 10 '16 at 05:10
  • @ScaryWombat I want to do setup on created object before any usage. Shouldn't I do this? I don't want to synchronize whole methods because I cleaned code of my methods for simplisity's sake – Alexey Andronov Aug 10 '16 at 05:14
  • You could improve the readability by making it legal Java, for starters. Are your methods part of a class? Are they supposed to be part of `MyClass`? What's `m_channelLock`? Regardless, I don't see anything uglier than normal about your code, where synchronization is concerned. Since I don't know what `m_channelLock` is, I can't tell whether you can simplify things any. – ajb Aug 10 '16 at 05:16
  • 1
    In `setChannel` as it is already locked, there is no point in `locking` again. – Scary Wombat Aug 10 '16 at 05:16
  • @ScaryWombat I don't think that's necessarily true. Other code could synchronize on `channel` without synchronizing on `lock` first, so the nested `synchronize` may be necessary. Unfortunately, since there are errors in the posted code, I can't tell for sure whether it makes a difference. – ajb Aug 10 '16 at 05:20
  • @ajb Yes, you are right if we are not assuming that `m_channelLock` is a typo and he means `lock` – Scary Wombat Aug 10 '16 at 05:29
  • @ajb it was a typo, sorry. I came from a c++ background and there we have LockGuards oneliners, so I'm not used to those java staircases in simplemethods like `isSet()` here (6 lines, really?) :) – Alexey Andronov Aug 10 '16 at 05:35
  • 1
    If `isSet()` really synchronizes on `lock`, then Scary Wombat is right and there's no reason to synchronize on `channel`, because the lock on `lock` will already prevent any other code from getting to the point where it tries to access `channel`. – ajb Aug 10 '16 at 05:48
  • @ajb the guy [here](http://stackoverflow.com/questions/6910807/synchronization-of-non-final-field/21462631#21462631) said you should syncronize on protected object "for the sake of RAM synchronisation", who is right then? – Alexey Andronov Aug 10 '16 at 08:05
  • 1
    I think you could get the same effect by declaring `channel` to be `volatile`, instead of synchronizing on it. The person at your link seemed to say that you have to declare _all_ instance fields to be volatile, but I don't think that's correct--only the ones that need to be. If you have a case where multiple threads may be accessing the same field (and some could be writes), then it should be `volatile`. – ajb Aug 11 '16 at 03:40
  • @ajb, thanks, I appreciate your comments, `volatile` it is – Alexey Andronov Aug 11 '16 at 04:33

2 Answers2

2

You can simplify your locking strategy:

class MyClass {
    private final Object lock = new Object();
    private Channel channel = null;

    public void setChannel() {
        // other code can go here

        synchronized (lock) {
            channel = new Channel();
            // setup channel
        }

        // other code can go here
    }

    public void unsetChannel() {

        // other code can go here

        synchronized (lock) {
            channel.close();
            channel = null;
        }

        // other code can go here
    }

    public boolean isSet() {
        synchronized (lock) {
            if (channel == null) {
                return false;
            }
            return channel.isActive();
        }
    }
}

In effect, the lock object protects any access to the channel variable.

Edited to show where other code that does not interact with channel might live outside of the locks.

Jason
  • 11,744
  • 3
  • 42
  • 46
  • the guy [here](http://stackoverflow.com/a/21462631/4440694) said you should syncronize on protected object "for the sake of RAM synchronisation". So whom should I believe? :) – Alexey Andronov Aug 10 '16 at 05:26
  • 1
    That only applies if your lock object is non-final. In my example, I am locking the final object `lock`. – Jason Aug 10 '16 at 05:28
  • i want to protect non-final "channel" object, so is your solution eligible? – Alexey Andronov Aug 10 '16 at 05:38
  • 1
    If you wrap every access to the `channel` object with the lock as I have above, the solution will work. – Jason Aug 10 '16 at 23:54
0

The outer lock on lock protects everything. You don't need the second lock. A second thread can never reach it while the outer lock is held.

user207421
  • 305,947
  • 44
  • 307
  • 483
  • the guy [here](http://stackoverflow.com/questions/6910807/synchronization-of-non-final-field/21462631#21462631) said you should syncronize on protected object "for the sake of RAM synchronisation". Is one lock is really enough? – Alexey Andronov Aug 10 '16 at 08:14