Can two threads access same method at once after access of synchronized property?-CodePudding

I am little puzzled by some situations in my code.

private var state by synchronized<PlayerState>(Stopped())

val asStarted get() = state as? Started
val asStopped get() = state as? Stopped

inner class Stopped : PlayerState {
    fun start(bpm: BPM = BPM(), volume: Int? = null) {
        state = Started(bpm, volume)
    }
}

Imagine two thread are trying to execute player.asStopped?.start(), do I have issue here? Do I need to enter something like this?

inner class Stopped : PlayerState {
    @Synchronized
    fun start(bpm: BPM = BPM(), volume: Int? = null) {
        if(state is Started) return
        state = Started(bpm, volume)
    }
}

Help is appreciated.

CodePudding user response：

You are right to be concerned.

The first version of your code does indeed have a race condition, as two threads could both create Started objects, and then set them. Only one thread can be executing the setter at a time, but the second can enter it as soon as the first leaves.

That may not be a problem here. (The synchronized delegate will still prevent any low-level problems with accessing the state, such as seeing partially-constructed values. And it looks like there's no other state which needs to be correlated with your state property, so it can't get into an inconsistent state.) But being able to start twice may not be good!

I don't think the second version of your code suffers from any threading issues, as written. (If it didn't have the @Synchronized, then it would indeed have a time-of-check-to-time-of-use race condition.)

However, it's a little confusing because it uses two separate locking mechanisms (the @Synchronized method locks on the Stopped object, while the by synchronized state has its own lock), and it's hard to follow the way those locks might interact, even in this very simple example.

I also suspect it could lead to race conditions in the surrounding code. Once started, can your player be stopped again? If so, what will the stop() method lock on — if it's the Started object, then that's yet another lock, and I can see some race conditions there.

So I'd strongly look at refactoring this to use only a single lock.

I don't know the synchronized delegate your code is using. If it has some sort of test-and-set, compare-and-swap, or similar function, then you may be able to do what you need using that. Otherwise, it's probably best to drop the delegate and do your own locking, I'm afraid.

This does raise a wider question about the design, though: why does your Stopped class have a start() method?? Surely it's the player which starts and stops, and the state is just a reflection of that? So changing the state isn't an operation on the current state at all.

Addressing that design issue will almost certainly make it easier to fix the locking too.