[asterisk-dev] [Code Review] res_stun_monitor module

[Simon Perreault]

> On 2010-08-11 13:40:51, Kevin Fleming wrote:
> > /trunk/res/res_stun_monitor.c, line 77
> > <https://reviewboard.asterisk.org/r/854/diff/2/?file=12184#file12184line77>
> >
> >     Should we be concerned about any packets that may have arrived on the socket while the scheduler was waiting to execute this request? If there are any, when the STUN query is sent out, there will be an (apparent) immediate response, and one of the queued packets will be read. It's possible ast_stun_request() already purges the socket of any queued incoming packets, I haven't looked... but if it doesn't, this is probably worth addressing.
> 
> David Vossel wrote:
>     the STUN API does not do this. I will purge the socket before issuing a new request.
> 
> Simon Perreault wrote:
>     Ah I understand now why you're doing that. But it's still wrong. If a packet arrives after you're done purging and before you issue a request, you're still screwed.
>     
>     What you need to do is two independent things:
>     
>     a. Send STUN Binding requests every X seconds.
>     b. Any time a response arrives on the socket, check for an address/port change, and potentially fire an event.
>     
>     These two things can be done completely independently. You don't need to wait for a response, and you don't need to associate a response with the request that triggered it.
> 
> David Vossel wrote:
>     I'm not sure how this is a problem.  If a STUN request somehow arrives for a previous request between purging and issuing a new request (which realistically wouldn't happen if there was any reasonable refresh timeout period used), then we just read the one that sneaked in.  Either way it is a response to a STUN request coming from the address we connected to, and any responses that came from the new request will get purged the next time around.
>     
>     I agree that your suggestion of breaking the requests and responses up makes the most sense, but the coupling of these actions is a current limitation of Asterisk's STUN API.  Unless this will somehow functionally not work, I don't think it should be necessary to start thinking about breaking up the code in stun.c.
> 
> Simon Perreault wrote:
>     I thought this was why you were purging the socket. So now can you explain why you're doing it? This socket purging function raises a red flag in my mind, and I'd really like to understand why it exists.
> 
> David Vossel wrote:
>     Asterisk's STUN API works like kind of like this.
>     
>     --------------------------------------
>     ast_stun_request(socket, destination)
>         while retry < max_retries
>              send request
>              wait for response
>              if response
>                  break;
>              else
>                  retries++
>     --------------------------------------
>     
>     We break out of that loop on the first UDP packet received.  Since it is possible we may send multiple requests as a result of a timeout we can't be guaranteed multiple responses will not come back.  So, the next time around when we are going to use the STUN api any messages that may be left on the receive buffer are removed.  This is just to clean up any previous responses we are not interested in anymore.
>     
>     I looked further into the STUN api documentation and it does appear to be possible to de-couple the sending and receiving from ast_stun_request, but I am not convinced changing the current architecture of this module to do that is necessary.

Makes sense. Still, please add an ugly and obvious comment above the purging function stating that this is a hack and that a better solution would be to change the architecture of the STUN API. Maybe add a reference to this discussion.


- Simon


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


On 2010-08-11 16:09:23, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/854/
> -----------------------------------------------------------
> 
> (Updated 2010-08-11 16:09:23)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The res_stun_monitor module monitors the perceived external ip/port provided by a STUN server and sends out an event when that perceived ip/port changes in any way.  This patch adds chan_sip and chan_iax as listeners to this event.  Both of these channel drivers react to the event by regenerating all outbound registers in order to alert the endpoints being registered with to the new address location.  This feature's primary use case is when Asterisk is behind a NAT in which the external ip address is dynamic or the port mappings of the NAT device are subject to change.
> 
> I will update the CHANGES file before committing this.  That is not included within this review though.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_iax2.c 281761 
>   /trunk/channels/chan_sip.c 281761 
>   /trunk/configs/iax.conf.sample 281761 
>   /trunk/configs/res_stun_monitor.conf.sample PRE-CREATION 
>   /trunk/configs/sip.conf.sample 281761 
>   /trunk/include/asterisk/event_defs.h 281761 
>   /trunk/res/res_stun_monitor.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/854/diff
> 
> 
> Testing
> -------
> 
> I tested all the res_stun_monitor.conf options to verify functionality.
> I tested the channel driver's event listening by triggering an address change and verifying registrations are sent via wireshark.
> 
> 
> Thanks,
> 
> David
> 
>