The `AbstractCloseable.doCloseImmediately()` race condition

[artembilan]

Version

2.15.0

Bug description

The code there is like this:

    protected void doCloseImmediately() {
        closeFuture.setClosed();
        state.set(State.Closed);
    }

So, we fulfill CloseFuture first, and then set the state to Closed.
This way Closeable.close(Closeable closeable) exits earlier than we mark that Closeable as Closed.

Actual behavior

In my case the unit test is failing sporadically for this:

session.close();
assertThat(clientSession.isClosed()).isTrue();

Expected behavior

The AbstractCloseable.doCloseImmediately() has those expressions swapped:

    protected void doCloseImmediately() {
        state.set(State.Closed);
        closeFuture.setClosed();
     }

Relevant log output

Other information

No response

[tomaswolf]

Not sure. Depends on when the session is considered "closed". The current code seems to assume the session is fully closed once all the listeners on the closeFuture have run, and apparently there may be asynchronous things going on. So maybe an even more correct way to set this state would be

closeFuture.addListener(f -> state.set(State.Closed));
closeFuture.setClosed();

However, ClientSessionImpl.waitFor(...) considers the session closed once closeFuture.isClosed(). So perhaps switching the order like you suggest would be a way. It's a risky change, though, since AbstractCloseable is used for different things, not just sessions.

[tomaswolf]

BTW, and unrelated to this: do you have tests for shared SSH session usage? We had issue #527 a while back, and it looked to me as if a shared SSH session might get closed too early; c.f. #527 (comment) .

[artembilan]

Well, the ClientSessionImpl leads to this:

    @Override
    public final boolean isClosed() {
        return state.get() == State.Closed;
    }

So, calling the ClientSessionImpl.close() might not report the proper state because it waits only for CloseFuture.

It is risky to keep it like that.
Similar race conditions may happen in other places.

I guess the complexity of the logic is because there is that third Graceful state.
Well, with that we got two new (Graceful, Immediate) instead of Opened, Closed before, but that's different story.

Thanks for pinging me about that shared session problem!
Will look into that today.