[reSIProcate] Question about ClientSubscription and onRequestRetry

Fri Aug 2 20:02:17 CDT 2013

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

On Thu, Aug 1, 2013 at 8:04 PM, Francis Joanis <francis.joanis at gmail.com>wrote:

> Hi again,
>
> I've attached a preliminary test (testSubscribe.cxx) which should only
> work on Unix for now - I will properly add it as a Windows one as well once
> I get there.
>
> It essentially tests three things: the normal subscription success case
> (which I broke) and the "application requests a retry after a failure" one
> for both a Timer F failure and a 408 response.
>
> The more I look at it, the more it feels like the second check (in the if
> statement), the one that looks at AppDialogSet::isReUsed(), should not even
> be there... However I'm still not 100% about this.
>
> Was there ever a need to prevent calling onNewSubscription (UAC) when the
> AppDialogSet was re-used? The code that ends up setting the mIsReUsed flag
> to true is pretty much only ClientSubscription::reSubscribe(). Then,
> looking at what calls reSubscribe (which is also public), we can see that
> this calling code also has a few checks around mOnNewSubscriptionCalled.
> This flag is also the first condition of the puzzling if statement at
> ClientSubscription.cxx:91... so I'm questioning whether that if statement
> could only check that first flag and not the second one.
>
> Anyway, I'll keep thinking about it tonight and, worst case, we can always
> revert my change until we find the final solution.
>
> Cheers,
> Francis
>
>
>
>
> On Thu, Aug 1, 2013 at 3:04 PM, Francis Joanis <francis.joanis at gmail.com>wrote:
>
>> Hi Jeremy/guys,
>>
>> Sorry for the inconvenience this is causing; looking at it again it is
>> pretty obvious that it is causing your problem and I should not have missed
>> it. I'll create a new "unit" test for this that exhibits both the normal
>> case and the one my original patch attempted to fix. Once that's done we'll
>> be able to properly fix the second case.
>>
>> I'll keep you posted.
>>
>> Regards,
>> Francis
>>
>> On 2013-08-01, at 1:45 PM, Jeremy Geras <jgeras at counterpath.com> wrote:
>>
>> We're in the middle of re-basing from the latest release, and found that
>> this change by Francis prevents onNewSubscription from getting called at
>> all in the normal case (UAC sends the SUBSCRIBE, UAS responds as normal).
>>
>>
>> Are we missing something?
>>
>>
>> Thanks,
>> Jeremy
>> ------------------------------
>> *From:* resiprocate-devel-bounces at resiprocate.org [
>> resiprocate-devel-bounces at resiprocate.org] on behalf of Scott Godin [
>> sgodin at sipspectrum.com]
>> *Sent:* Wednesday, April 17, 2013 2:19 PM
>> *To:* Francis Joanis
>> *Cc:* resiprocate-devel
>> *Subject:* Re: [reSIProcate] Question about ClientSubscription and
>> onRequestRetry
>>
>> Sounds right to me - the mOnNewSubscriptionCalled check will ensure we
>> don't call onNewSubscription twice.
>>
>> Scott
>>
>> On Wed, Apr 17, 2013 at 4:03 PM, Francis Joanis<francis.joanis at gmail.com>
>>  wrote:
>>
>>> Hi guys,
>>>
>>> For the following scenario:
>>>
>>> - DUM UAC sends SUBSCRIBE to UAS
>>> - UAS isn't responding and UAC times out
>>> - On the UAC, onRequestRetry gets called and returns 0 (to retry ASAP)
>>> - DUM retries
>>> - UAS replies as usual (normal SIP flow)
>>>
>>> In that case, we would expect onNewSubscription(client) to be called
>>> when the NOTIFY comes in once the 2nd SUBSCRIBE works but it doesn't...
>>>
>>> It looks like the following line (91) in ClientSubscription.cxx:
>>>
>>> if(!mOnNewSubscriptionCalled && !getAppDialogSet()->isReUsed())
>>>
>>> should actually read
>>>
>>> if(!mOnNewSubscriptionCalled && getAppDialogSet()->isReUsed())
>>>
>>> without the ! in front of the second check.
>>>
>>> I made a quick test and it looks like it resolves the issue, but I
>>> wanted to run it by you guys before submitting a fix. What do you think?
>>>
>>> Thanks,
>>> Francis
>>>
>>> _______________________________________________
>>> resiprocate-devel mailing list
>>> resiprocate-devel at resiprocate.org
>>> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20130802/7c88951f/attachment.htm>