Subscriptions are not terminated on the client when the maxInterval expires

[bzbarsky-apple]

Problem

This got broken by #20080.

In RefreshLivenessCheckTimer we used to do:

System::Clock::Timeout timeout = System::Clock::Seconds16(mMaxInterval) + mExchange->GetSessionHandle()->GetAckTimeout();

but this got changed to:

        VerifyOrReturnError(mReadPrepareParams.mSessionHolder, CHIP_ERROR_INCORRECT_STATE);
        timeout = System::Clock::Seconds16(mMaxInterval) + mReadPrepareParams.mSessionHolder->GetAckTimeout();

When not doing resubscription, mReadPrepareParams doesn't contain anything, so that VerifyOrReturnError always fails. But nothing ever checks the return value of RefreshLivenessCheckTimer, so we proceed with things. The timer is not scheduled, and if we never hear back from the other side we don't treat that as a subscription failure.

Proposed Solution

1. Add tests that would have caught this problem. It's fairly surprising that we don't have any.
2. Figure out whether callers should be checking the return value of RefreshLivenessCheckTimer. If not, it should return void.
3. Decide whether lack of a session here should lead to an immediate Close call, just like failure to schedule a timer does.
4. Figure out where the session should be coming from here:
   • Possibly we should go back to using the exchange (since all the non-test calls to this function do in fact have an exchange), and if we need OverrideLivenessTimeout to work we can fall back to the read prepare params if there is no exchange.
   • As above, but change OverrideLivenessTimeout to just take a session as argument.
   • Another option would be to always store the session in mReadPrepareParams, even when we're not doing auto-resubscription. But then we'd also want to update it any time we get a message, because the incoming message may not be on the session we sent the original subscribe on. In fact, that session could be long-gone even as we're getting messages from the server on a different session, so getting a session from mReadPrepareParams is just not at all reliable.

@mrjerryjohns @yunhanw-google

[mrjerryjohns]

Add tests that would have caught this problem. It's fairly surprising that we don't have any.

I'm surprised too. Will add.

Figure out whether callers should be checking the return value of RefreshLivenessCheckTimer. If not, it should return void.

There's clearly precedence for calling Close() to handle errors in there, so we should be using that and switching this to void.

Possibly we should go back to using the exchange (since all the non-test calls to this function do in fact have an exchange), and if we need OverrideLivenessTimeout to work we can fall back to the read prepare params if there is no exchange.

That would make OverrideLivenessTimeout not usable in a capacity where re-subs are disabled.

As above, but change OverrideLivenessTimeout to just take a session as argument.

This seems onerous, especially when the session info could already be made available in private state.

Another option would be to always store the session in mReadPrepareParams, even when we're not doing auto-resubscription. But then we'd also want to update it any time we get a message, because the incoming message may not be on the session we sent the original subscribe on. In fact, that session could be long-gone even as we're getting messages from the server on a different session, so getting a session from mReadPrepareParams is just not at all reliable.

Yes, this is what I'm leaning to.

[mrjerryjohns]

There's clearly precedence for calling Close() to handle errors in there, so we should be using that and switching this to void.

I take that back - it should return an error, since it's being called during various inbound message processing flows where the final closure of the client object is handled somewhere else. The logic to call Close() within that method should be removed, and moved up to OverrideLiveness...