[asterisk-dev] [Code Review] 3699: RLS: Inbound SUBSCRIBE handling

[Matt Jordan]

> On July 3, 2014, 7:55 a.m., Matt Jordan wrote:
> > /team/group/rls/res/res_pjsip_pubsub.c, line 772
> > <https://reviewboard.asterisk.org/r/3699/diff/1/?file=61763#file61763line772>
> >
> >     Thinking about the error conditions here:
> >     * If the new_subscribe handler returned the SIP response code as an error, that could be returned from this function.
> >     * The recursive calls to this function would return the result if non-zero
> >     * That _may_ then allow build_resource_tree to return a more specific error condition if something goes wrong when building out the tree
> 
> Mark Michelson wrote:
>     These are good thoughts, but in practice they're difficult to execute. Let's say I have a resource list with three resources: alice, bob, and carol.
>     
>     new_subscribe() on alice returns a 404 since the alice resource can't be found.
>     new_subscribe() on bob returns a 500 due to some internal error.
>     new_subscribe() on carol returns a 485 due to ambiguity.
>     
>     What does build_node_children() return in this case? It doesn't seem possible to distill multiple errors down to a single response code.

Ah. Your subsequent finding explains why this is a giant pain.

I had thought that any error in subscription would cause the entire subscription to fail. If that were the case, then you wouldn't need to distill multiple errors down. You could bail on the first error returned however. Using your example, if new_subscribe() on alice returns a 404, then we don't event attempt a new_subscribe() on bob or carol; the routine returns a 404 which pops up through the recursion.

Unfortunately, if alice returns a 404 and bob returns a 500, but carol returns a 200, we are apparently supposed to succeed. That's kind of unfortunate, but probably also necessary from a usability perspective.

If that is the case, then we could still do something like this:
 - If any node succeeds, then we return 200
 - If all nodes failed, then return the highest error cause code

Generally, I would consider a 5xx error to be "worse" than a 4xx error. I'm not sure you can make equivalence claims for errors within a particular class (would we rather see a 484 or 404)?

Does the RFC say anything about what to do with this with respect to individual resources within a list?


- Matt


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3699/#review12463
-----------------------------------------------------------


On July 1, 2014, 6:21 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3699/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 6:21 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23868
>     https://issues.asterisk.org/jira/browse/ASTERISK-23868
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This modifies res_pjsip_pubsub to be able to handle inbound SUBSCRIBE requests that set up resource list subscriptions. The main gist of this is creating the tree of ast_sip_subscription structures that will represent the resource list. The changes can be broken into the following categories:
> 
> * Code that gets rid of assumptions that all subscriptions are real subscriptions. Now, code should work equally well with real and virtual subscriptions.
> * Code that has been moved within the file. These are functions that previously were used by only a single caller but now are used by multiple callers. Therefore, the functions have been moved closer to the top of the file so that they may be referenced throughout.
> * Code that builds a resource tree. A resource tree is essentially the front line of handling a resource list SUBSCRIBE. When the SUBSCRIBE is handled the requested resource is looked up, and a tree is built based on the resources in the list. The tree is built by asking subscription handlers if the individual resources in the tree can be subscribed to. Those that can be subscribed to are added to the tree. In addition, as the tree is being built, duplicated resource names and looped lists are eliminated.
> * Code that builds a subscription tree. Once a resource tree is built, it is used to create a tree of ast_sip_subscriptions. It is this tree of subscriptions that will be used throughout the life of the SUBSCRIBE dialog to build notification bodies.
> * XXX comments indicating recognized problem spots that are to be addressed in created ASTERISK issues.
> * Unit tests that check the resource tree algorithms.
> 
> For the most part, I'm satisfied with this change set. The only thing I don't really like is that when subscribing to a resource list, the responses that Asterisk currently can send are either a 200 or 500. I could not think of an elegant way to give more specific responses (e.g. 482 when subscribing to a list that resulted in a loop).
> 
> 
> Diffs
> -----
> 
>   /team/group/rls/res/res_pjsip_pubsub.c 417733 
> 
> Diff: https://reviewboard.asterisk.org/r/3699/diff/
> 
> 
> Testing
> -------
> 
> Given that NOTIFY handling and an rlmi+xml body generator have not yet been written, I can't test this with testsuite tests yet. The best that I could do was to create unit tests for the resource tree algorithms. The tests verify that the resource trees are being built as expected, and they verify that off-nominal test cases (duplicated resources, loops, lists of bad resources) behave as expected. This lends some confidence to the idea that the front line is working, at least.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140703/0738084e/attachment-0001.html>