[Asterisk-code-review] chan_psip, res_pjsip_sdp_rtp: ignore rtptimeout if direct-media is ac... (asterisk[16])

[Michael Neuhauser]

Michael Neuhauser has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13860 )

Change subject: chan_psip, res_pjsip_sdp_rtp: ignore rtptimeout if direct-media is active
......................................................................


Patch Set 1:

> Patch Set 1:
> 
> Just commenting here, I'm exploring and brainstorming to see if there's other options for improving this as I always like to reduce channel locking when possible instead of increasing it, unless there are no better options.

I'm not sure what "better options" you are thinking about (scope wise).

Thread A modifies memory locations X and Y while holding lock L (check_for_rtp_changes() in chan_pjsip.c). Thread B (timer, rtp_check_timeout() in res/res_pjsip_sdp_rtp.c) reads those memory locations, potentially at the exact same moment as thread A is modifying them, so it MUST acquire the same lock as thread A before accessing X and Y to see "stable" values and have a deterministic behaviour. (At least using only POSIX stuff, I'm assuming that we do not want to build our own atomic primitives library based on GCC builtins ...)

Granted, the probability that RTP is returning to Asterisk at the exact same moment as a RTP timeout check is running is low - until same automated thing has a connection time that is per chance exactly the same as the RTP-timeout and suddenly it is much more likely to happen. So I think the solution should cover event this corner case.

Also, the RTP timeout is on the order of multiple seconds. I don't see the problem locking the channel every 5 seconds for a quick timeout check.

The best I can think of: first, check the timeout as it is now in, i.e., unguarded by the channel lock. If that check says that there *is* a timeout, only then lock the channel and check the timeout (again) and also the direct media situation while holding the lock. This guarantees no false positives and the channel is only locked when it is locked now (on timeout).


-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13860
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I8b62012be7685849e8fb2b1c5dd39d35313ca2d1
Gerrit-Change-Number: 13860
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Neuhauser <mike at firmix.at>
Gerrit-Reviewer: Friendly Automation
Gerrit-CC: Joshua Colp <jcolp at sangoma.com>
Gerrit-CC: Sean Bright <sean.bright at gmail.com>
Gerrit-Comment-Date: Mon, 16 Mar 2020 16:19:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200316/dbca7114/attachment-0001.html>