[QUESTION] Thread safe problem about HandlerState#changeToReadyState

[BewareMyPower]

I'm not sure if it's bug. It's more a question. As we can see,

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/HandlerState.java

Lines 53 to 56 in 6089292

	protected boolean changeToReadyState() {
	return (STATE_UPDATER.compareAndSet(this, State.Uninitialized, State.Ready)
	|| STATE_UPDATER.compareAndSet(this, State.Connecting, State.Ready)
	|| STATE_UPDATER.compareAndSet(this, State.RegisteringSchema, State.Ready));

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

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java

Lines 738 to 739 in 6089292

	synchronized (ConsumerImpl.this) {
	if (changeToReadyState()) {

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.

pulsar/pulsar-client/src/main/java/org/apache/pulsar/client/impl/TransactionMetaStoreHandler.java

Lines 115 to 117 in 6089292

	if (!changeToReadyState()) {
	cnx.channel().close();
	}

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?

[BewareMyPower]

Is following implementation better?

    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;
    }

[codelipenghui]

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