Hi,
In order to demystify the if condition @ line 91 of ClientSubscription.cxx I went back and studied the code a bit more. This is what the "if" looked like before my change:
if (!mOnNewSubscriptionCalled && !getAppDialogSet()->isReUsed())
The first part of the "if" is fairly obvious. The mOnNewSubscriptionCalled flag is, in a nutshell, set to true once the first NOTIFY request is received by the UAC. This conforms with the the RFC which says that the UAC's subscription actually starts once the first NOTIFY is received - it can even be received before the 202 Accepted for the SUBSCRIBE if the messages cross on the wire, so this makes sense. Once the onNewSubscription callback is called on the handler, then mOnNewSubscriptionCalled is set to true.
The second flag is the one that is puzzling me. The check for isReUsed() is tied to the usage of the ClientSubscription::reSubscribe() method because this is the only method that ends up setting isReUsed() so that it returns true.
ClientSubscription::reSubscribe() is called by the code when DUM and/or the application decide that a UAC SUBSCRIBE failure
can be automatically retried without any "higher level" interaction (in
other words, "automatically"). This can happen when a SUBSCRIBE failure
happens with some error codes and a Retry-After header.
What actually happens when reSubscribe() is called is: 1) the
AppDialogSet is "reused" and 2) the current ClientSubscription is
destroyed (by calling "delete this"). This means that a new ClientSubscription usage will end up being created. Since this new subscription is new it won't have "remembered" the mOnNewSubscriptionCalled value from its previous life. This then means that when the troublesome "if" condition will be reached again (by the new subscription) it will always have mOnNewSubscriptionCalled set to false.
So, the condition does look like it has been put there to prevent calling onNewSubscription more than once per UAC "multi-usage" subscription: since mOnNewSubscriptionCalled will be set to false the code needs to then check whether the AppDialogSet was "reused" or not. In that case it would have been reused and the code within the "if"'s block would not have been called. So far so good... but there are still things that are not clear to me.
First, there are use cases that pretty much prevent onNewSubscription from being called at all. For example, if the UAC SUBSCRIBE gets black holed the stack will generate a locally generated 408 response. If then ClientSubscriptionHandler::onRequestRetry returns 0 then the code will "reuse" the AppDialogSet before onNewSubscription was called (since no NOTIFY came in). The end result is that the "if" statement doesn't validate and then no onNewSubscription callback is ever called. This is the bug I was trying to fix.
Second, if there are use cases where it would be possible to have onNewSubscription called first then have the AppDialogSet "reused", then I wouldn't see the point of actually hiding the subsequent onNewSubscription callbacks. For example, this could happen if the UAS, for whatever reason, would send both an error (like 481 w/ Retry-After) and a NOTIFY and those two would cross on the wire so that the NOTIFY is received by the UAC before the 481 - I should actually add this to my "unit" test...
Anyway, for that case how would the application know that the ClientSubscriptionHandle has changed if no subsequent onNewSubscription is called? I don't even think it might get an onTerminated for the first subscription but I will create a new test to confirm this.
I'll try to map out the reSubscribe() use cases and to add them to my tests.
In the mean time I will revert my change on the mainline and I'll also send an email to the resiprocate-users list as a heads-up for the bug.
Should we also revert my change on the latest 1.8.x branch (and possibly re-generate a tarball)?
Thanks,
Francis