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

[GitBox <gi...@apache.org>]

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-unsubscribe@pulsar.apache.org

[GitHub] [pulsar] BewareMyPower added a comment to the discussion: [QUESTION] Thread safe problem about HandlerState#changeToReadyState

[GitBox <gi...@apache.org>]

GitHub user BewareMyPower added a comment to the discussion: [QUESTION] Thread safe problem about HandlerState#changeToReadyState

Is following implementation better?

```java
    private static boolean notClosed(State state) {
        return state == State.Uninitialized || state == State.Connecting || state == State.RegisteringSchema;
    }

    // moves the state to ready if it wasn't closed
    protected boolean changeToReadyState() {
        return STATE_UPDATER.getAndUpdate(this, state -> (notClosed(state) ? State.Ready : state)) == State.Ready;
    }

```

GitHub link: https://github.com/apache/pulsar/discussions/18401#discussioncomment-4103573

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

[GitHub] [pulsar] codelipenghui added a comment to the discussion: [QUESTION] Thread safe problem about HandlerState#changeToReadyState

[GitBox <gi...@apache.org>]

GitHub user codelipenghui added a comment to the discussion: [QUESTION] Thread safe problem about HandlerState#changeToReadyState

The issue had no activity for 30 days, mark with Stale label.

GitHub link: https://github.com/apache/pulsar/discussions/18401#discussioncomment-4103574

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