GitHub user BewareMyPower created a discussion: [QUESTION] Thread safe problem 
about HandlerState#changeToReadyState

I'm not sure if it's bug. It's more a question. As we can see, 
https://github.com/apache/pulsar/blob/608929227824fe4303f46aa432e42af77bcbf625/pulsar-client/src/main/java/org/apache/pulsar/client/impl/HandlerState.java#L53-L56

`HandlerState#changeToReadyState` is not an atomic operation. I'm not sure 
there's a race case like following timeline

| Time | Event | State Before | State Now |
| :---- | :----- | :------------ | :---------- |
| 1 | `STATE_UPDATER.compareAndSet(this, State.Uninitialized, State.Ready)` | 
`State.Connecting` | `State.Connecting` |
| 2 | `setState(State.Uninitialized)` | `State.Connecting` | 
`State.Uninitialized` |
| 3 | `STATE_UPDATER.compareAndSet(this, State.Connecting, State.Ready)` | 
`State.Uninitialized` | `State.Uninitialized` |
| 4 | `STATE_UPDATER.compareAndSet(this, State.RegisteringSchema, State.Ready)` 
| `State.Uninitialized` | `State.Uninitialized` |

As we can see, there's a time point that the state was changed back to 
`Uninitialized` from `Connecting`. However, we should expect the state to be 
`Ready` because neither `Uninitialized` nor `Connecting` was a closed state.

I see references of `changeToReadyState` in `ProducerImpl` and `ConsumerImpl` 
were protected by the lock directly or indirectly, like 
https://github.com/apache/pulsar/blob/608929227824fe4303f46aa432e42af77bcbf625/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java#L738-L739

I'm not sure if the lock works because it requires some `setState` invocations 
are protected by the lock and I didn't check it in detail.

And in `TransactionMetaStoreHandler#connectionOpened`, there's no lock.

https://github.com/apache/pulsar/blob/608929227824fe4303f46aa432e42af77bcbf625/pulsar-client/src/main/java/org/apache/pulsar/client/impl/TransactionMetaStoreHandler.java#L115-L117

I'm not sure if the thread safety could be guaranteed. IMO, if there's no 
possibility that the state was changed back to `Connecting` or `Uninitialized` 
during `changeToReadyState`, it will be thread safe. Or this race condition is 
acceptable?

GitHub link: https://github.com/apache/pulsar/discussions/18401

----
This is an automatically sent email for dev@pulsar.apache.org.
To unsubscribe, please send an email to: dev-unsubscr...@pulsar.apache.org

Reply via email to