[Asterisk-code-review] chan sip: Don't unload module while TCP/TLS threads are stil... (asterisk[master])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/4536 )

Change subject: chan_sip: Don't unload module while TCP/TLS threads are still running
......................................................................


Patch Set 2: Code-Review-1

> This should only lead to an infinite loop if there is another bug
 > that prevents a TCP thread from terminating, and in that case, the
 > bug would need to be fixed instead of just keeping a stuck thread
 > around (and possibly piling them up over the run-time of Asterisk).
 > In theory, a TCP thread should only ever be "stuck" on a blocking
 > syscall like connect(), and that is interrupted by the SIGURG. I
 > have now added pthread_kill(..., SIGURG) to that 5-second loop so
 > that it repeatedly tries to terminate the thread by interrupting
 > its blocking syscalls.
 > 
 > Preventing dlclose as you suggested may be an alternative, but if
 > the thread uses any global resources that are cleaned up later
 > during the unload, we could still get a crash.

Preventing dlclose will prevent a crash during shutdown.  Failure to cleanup any module prevents cleanup of global resources in the Asterisk core.  This was fixed by ASTERISK-26513 in all active branches.  If chan_sip is using other modules, it is expected to hold a reference to the other module.

Please take a look a 'man pthread_mutex_lock'.  It states "These functions shall not return an error code of [EINTR]."  I interpret this to mean that pthread_mutex_lock cannot be interrupted by pthread_kill since EINTR is the correct error code to use in that situation.  Thus if two threads are deadlocked in pthread_mutex_lock they will never exit.

-- 
To view, visit https://gerrit.asterisk.org/4536
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I23a8abf9e8f73b57ff7ada61984b8506aea6cbde
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Michael Kuron <m.kuron at gmx.de>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Michael Kuron <m.kuron at gmx.de>
Gerrit-HasComments: No