[asterisk-dev] [Code Review] 2611: Refactor skinny sessions

[Matt Jordan]

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


I think the biggest suggestion I would have in the future is to make the skinny_session an ao2 ref counted object. That would dramatically simplify some of the cleanup code you've had to write here, and simplify some of the locking semantics you've implemented. It's up to you whether or not you want to go down that road - it certainly isn't mandatory - but if the skinny_session were ref counted you could simply drop the reference count when the thread is done with it and move a good chunk of the pthread cleanup code to the session destructor. It would also implicitly provide a lock for the skinny_session object.

But that doesn't have to be done. The only major finding is to use the new Stasis message bus code to publish the change in the endpoint state as opposed to raising it directly in AMI.


/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17877>

    I know why you have this, but in general having to keep track of whether or not you have locked an object so that you properly unlock it before disposing of the object is usually a sign of a design flaw.
    
    As a future enhancement, you should consider making the skinnysession object ao2 ref counted. You can also use scoped AO2 locks (or just scoped locks in general) in trunk to help ensure that a locked object is unlocked when it leaves scope. That may help remove the need for this property.



/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17872>

    Blob



/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17880>

    You could reduce indentation by checking if l->device != d and continuing



/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17881>

    These have all been refactored now to be updates of a Stasis endpoint - see skinny_unregister for an example of publishing this message over the Stasis message bus



/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17879>

    Blob



/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17878>

    Blob



/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17875>

    Is there any reason to not initialize the lock in the skinny session object when it is first created?



/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17873>

    Blob



/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17876>

    Blob



/trunk/channels/chan_skinny.c
<https://reviewboard.asterisk.org/r/2611/#comment17874>

    Set the lockstate to 0 immediately before unlocking. Otherwise something else could obtain the lock, set the lockstate to 1, and you would then set the lockstate to 0 outside of the mutex.


- Matt Jordan


On June 9, 2013, 11:40 p.m., wedhorn wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2611/
> -----------------------------------------------------------
> 
> (Updated June 9, 2013, 11:40 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Refactor and cleanup of skinny session handling. Major changes are to pull all packet reading functions into skinny_session and move timeout handling to scheduling arrangements. Thread cancelling is now undertaken directly rather than waiting for the read to timeout (cleanup is popped on thread cancel). Also added some keepalive timings in debugging messages.
> 
> Also, keepalive timeout has been increased from 1.1 by keepalive to 3 times keepalive. This seems to align (after keepalives stabilise) with when devices reset after not receiving keepalives. Probably needs more work, especially around the first and/or second keepalives that vary significantly by device and firmware version.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_skinny.c 391061 
> 
> Diff: https://reviewboard.asterisk.org/r/2611/diff/
> 
> 
> Testing
> -------
> 
> Ongoing. Generally seems better and less issues with a graceful shutdown.
> 
> 
> Thanks,
> 
> wedhorn
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130706/19ec2e6d/attachment.htm>