[reSIProcate] Question about ClientSubscription and onRequestRetry

[Francis Joanis]

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/20130801/062dfa94/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: testSubscribe.cxx
Type: application/octet-stream
Size: 11393 bytes
Desc: not available
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20130801/062dfa94/attachment.obj>