[Asterisk-code-review] res pjsip: Refactor endpt send transaction (qualify timeout) (asterisk[13])

[Joshua Colp]

Joshua Colp has posted comments on this change.

Change subject: res_pjsip: Refactor endpt_send_transaction (qualify_timeout)
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

https://gerrit.asterisk.org/#/c/492/1//COMMIT_MSG
Commit Message:

Line 15: The original version of this feature used the lower level calls to
       : get access to the tsx structure in order to cancel the transaction
       : when our own timer expires. Since we no longer have that access,
       : if our own timer expires before the pjsip timer, we call the callbacks
       : and just let the pjsip transaction take it's own course.  When the
       : transaction ends, it discovers the callbacks have already been run
       : and just cleans itself up.
The documentation in res_pjsip.h needs to be updated with this, and this longer explanation of the behavior would be nice there as well.


https://gerrit.asterisk.org/#/c/492/1/res/res_pjsip.c
File res/res_pjsip.c:

Line 2944: 			/* If it wasn't cancelled, it MAY be in the callback already
         : 			 * waiting on the lock so set the id to INACTIVE so
         : 			 * when the callback comes out of the lock, it knows to not
         : 			 * proceed.
         : 			 */
If this scenario occurs are you guaranteed that the tdata pool you allocated from will remain valid? Does it hinge on the tdata remaining as part of the transaction after it has completed? Should the req_wrapper hold a reference to the tdata to guarantee it?


Line 2979: 	/* If the id is not TIMEOUT_TIMER2 then the timer was cancelled above
         : 	 * between the time the timer expired and we grabbed the lock so just clean up.
         : 	 */
         : 	if (entry->id != TIMEOUT_TIMER2) {
         : 		ast_debug(3, "%p: Timeout already handled\n", req_wrapper);
         : 		ao2_ref(req_wrapper, -1);
         : 		return;
         : 	}
Why have this here as well as after the lock?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0810f3999cf63f3a72607bbecac36af0a957f33e
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes