Re: [asterisk-dev] [Code Review] 4572: RFC: Refactor Qualify and res_pjsip/endpt_send_request
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Hi, Though I'm following ASTERISK-24863, I may have missed something: what is this change replaced by? Thanks, - -- Jean-Denis Girard SysNuxSystèmes Linux en Polynésie française http://www.sysnux.pf/ Tél: +689 40.50.10.40 / GSM: +689 87.79.75.27 Le 06/04/2015 04:46, George Joseph a écrit : This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/ This change has been discarded. Review request for Asterisk Developers. By George Joseph. /Updated April 6, 2015, 8:46 a.m./ *Bugs: * ASTERISK-24863 https://issues.asterisk.org/jira/browse/ASTERISK-24863 *Repository: * Asterisk Description This review is based on the discussion here http://lists.digium.com/pipermail/asterisk-dev/2015-March/073921.html - Right now when a contact is qualified, it's immediately marked as UNAVAILABLE. If a response to the OPTIONS message comes back, it's marked as AVAILABLE again and the round trip time calculated. The status flapping even in normal operation makes generating events tricky because the subscriber will see up/down events even if the contact is really AVAILABLE. Now the pjsip transaction will eventually time out at an unconfigurable 32 seconds and call the callback but 32 seconds is a long time. This is also an issue if the qualify_frequency is less than 32 seconds since they'll be multiple pjsip transactions in progress for the same contact. snip So, I'm proposing pulling libpjsip/pjsip_endpt_send_request up into res_pjsip. It's 2 short function and a pjsip_module structure with no access to any private pjsip stuff. Now we'll have the ability to terminate the transaction AND it seems that there's a timeout member of pjsip_transaction which I'm hoping (but haven't tested) will eliminate the need to add timeout processing in pjsip_options. The result of all of this is that we'll be able to generate events for contact status (ASTERISK-24863) which in turn will help is provide functionality like marking an endpoint unavailable when all of its contacts are unavailable. - Both Josh and Mark had comments in the thread so this is an RFC ONLY r eview. It covers the pull up of libpjsip/pjsip_endpt_send_request but I've al so added the use case which is creating and processing a new aor/contact option cal led 'qualify_timeout'. I'll split the final submission into 2 pieces. Testing All existing pjsip related tests complete. I'll also have to add even t generation on contact status change and the tests based on it. Diffs * branches/13/res/res_pjsip/pjsip_options.c (433794) * branches/13/res/res_pjsip/location.c (433794) * branches/13/res/res_pjsip.c (433794) * branches/13/include/asterisk/res_pjsip.h (433794) View Diff https://reviewboard.asterisk.org/r/4572/diff/ -BEGIN PGP SIGNATURE- iEYEARECAAYFAlUjAP8ACgkQuu7Rv+oOo/jHAACgrLQSb1fYFB3XEOSlGGATsXmD XA4AnR2EwCvc+fz6clwFeyF4Ep1uy4eg =H7uU -END PGP SIGNATURE- -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4572: RFC: Refactor Qualify and res_pjsip/endpt_send_request
On Mon, Apr 6, 2015 at 3:56 PM, Jean-Denis Girard jd.gir...@sysnux.pf wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Hi, Though I'm following ASTERISK-24863, I may have missed something: what is this change replaced by? Oops, I meant to update the ticket. These are the 2 real reviews... https://reviewboard.asterisk.org/r/4587/ https://reviewboard.asterisk.org/r/4585/ Thanks, - -- Jean-Denis Girard -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4572: RFC: Refactor Qualify and res_pjsip/endpt_send_request
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/#review15070 --- Hey George - is this review useful anymore since you've got the two others? - Joshua Colp On March 31, 2015, 5:57 a.m., George Joseph wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/ --- (Updated March 31, 2015, 5:57 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24863 https://issues.asterisk.org/jira/browse/ASTERISK-24863 Repository: Asterisk Description --- This review is based on the discussion here http://lists.digium.com/pipermail/asterisk-dev/2015-March/073921.html - Right now when a contact is qualified, it's immediately marked as UNAVAILABLE. If a response to the OPTIONS message comes back, it's marked as AVAILABLE again and the round trip time calculated. The status flapping even in normal operation makes generating events tricky because the subscriber will see up/down events even if the contact is really AVAILABLE. Now the pjsip transaction will eventually time out at an unconfigurable 32 seconds and call the callback but 32 seconds is a long time. This is also an issue if the qualify_frequency is less than 32 seconds since they'll be multiple pjsip transactions in progress for the same contact. snip So, I'm proposing pulling libpjsip/pjsip_endpt_send_request up into res_pjsip. It's 2 short function and a pjsip_module structure with no access to any private pjsip stuff. Now we'll have the ability to terminate the transaction AND it seems that there's a timeout member of pjsip_transaction which I'm hoping (but haven't tested) will eliminate the need to add timeout processing in pjsip_options. The result of all of this is that we'll be able to generate events for contact status (ASTERISK-24863) which in turn will help is provide functionality like marking an endpoint unavailable when all of its contacts are unavailable. - Both Josh and Mark had comments in the thread so this is an RFC ONLY review. It covers the pull up of libpjsip/pjsip_endpt_send_request but I've also added the use case which is creating and processing a new aor/contact option called 'qualify_timeout'. I'll split the final submission into 2 pieces. Diffs - branches/13/res/res_pjsip/pjsip_options.c 433794 branches/13/res/res_pjsip/location.c 433794 branches/13/res/res_pjsip.c 433794 branches/13/include/asterisk/res_pjsip.h 433794 Diff: https://reviewboard.asterisk.org/r/4572/diff/ Testing --- All existing pjsip related tests complete. I'll also have to add event generation on contact status change and the tests based on it. Thanks, George Joseph -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4572: RFC: Refactor Qualify and res_pjsip/endpt_send_request
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/ --- (Updated April 6, 2015, 8:46 a.m.) Status -- This change has been discarded. Review request for Asterisk Developers. Bugs: ASTERISK-24863 https://issues.asterisk.org/jira/browse/ASTERISK-24863 Repository: Asterisk Description --- This review is based on the discussion here http://lists.digium.com/pipermail/asterisk-dev/2015-March/073921.html - Right now when a contact is qualified, it's immediately marked as UNAVAILABLE. If a response to the OPTIONS message comes back, it's marked as AVAILABLE again and the round trip time calculated. The status flapping even in normal operation makes generating events tricky because the subscriber will see up/down events even if the contact is really AVAILABLE. Now the pjsip transaction will eventually time out at an unconfigurable 32 seconds and call the callback but 32 seconds is a long time. This is also an issue if the qualify_frequency is less than 32 seconds since they'll be multiple pjsip transactions in progress for the same contact. snip So, I'm proposing pulling libpjsip/pjsip_endpt_send_request up into res_pjsip. It's 2 short function and a pjsip_module structure with no access to any private pjsip stuff. Now we'll have the ability to terminate the transaction AND it seems that there's a timeout member of pjsip_transaction which I'm hoping (but haven't tested) will eliminate the need to add timeout processing in pjsip_options. The result of all of this is that we'll be able to generate events for contact status (ASTERISK-24863) which in turn will help is provide functionality like marking an endpoint unavailable when all of its contacts are unavailable. - Both Josh and Mark had comments in the thread so this is an RFC ONLY review. It covers the pull up of libpjsip/pjsip_endpt_send_request but I've also added the use case which is creating and processing a new aor/contact option called 'qualify_timeout'. I'll split the final submission into 2 pieces. Diffs - branches/13/res/res_pjsip/pjsip_options.c 433794 branches/13/res/res_pjsip/location.c 433794 branches/13/res/res_pjsip.c 433794 branches/13/include/asterisk/res_pjsip.h 433794 Diff: https://reviewboard.asterisk.org/r/4572/diff/ Testing --- All existing pjsip related tests complete. I'll also have to add event generation on contact status change and the tests based on it. Thanks, George Joseph -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4572: RFC: Refactor Qualify and res_pjsip/endpt_send_request
On April 6, 2015, 7:52 a.m., Joshua Colp wrote: Hey George - is this review useful anymore since you've got the two others? Nope. Will discard. - George --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/#review15070 --- On March 30, 2015, 11:57 p.m., George Joseph wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/ --- (Updated March 30, 2015, 11:57 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24863 https://issues.asterisk.org/jira/browse/ASTERISK-24863 Repository: Asterisk Description --- This review is based on the discussion here http://lists.digium.com/pipermail/asterisk-dev/2015-March/073921.html - Right now when a contact is qualified, it's immediately marked as UNAVAILABLE. If a response to the OPTIONS message comes back, it's marked as AVAILABLE again and the round trip time calculated. The status flapping even in normal operation makes generating events tricky because the subscriber will see up/down events even if the contact is really AVAILABLE. Now the pjsip transaction will eventually time out at an unconfigurable 32 seconds and call the callback but 32 seconds is a long time. This is also an issue if the qualify_frequency is less than 32 seconds since they'll be multiple pjsip transactions in progress for the same contact. snip So, I'm proposing pulling libpjsip/pjsip_endpt_send_request up into res_pjsip. It's 2 short function and a pjsip_module structure with no access to any private pjsip stuff. Now we'll have the ability to terminate the transaction AND it seems that there's a timeout member of pjsip_transaction which I'm hoping (but haven't tested) will eliminate the need to add timeout processing in pjsip_options. The result of all of this is that we'll be able to generate events for contact status (ASTERISK-24863) which in turn will help is provide functionality like marking an endpoint unavailable when all of its contacts are unavailable. - Both Josh and Mark had comments in the thread so this is an RFC ONLY review. It covers the pull up of libpjsip/pjsip_endpt_send_request but I've also added the use case which is creating and processing a new aor/contact option called 'qualify_timeout'. I'll split the final submission into 2 pieces. Diffs - branches/13/res/res_pjsip/pjsip_options.c 433794 branches/13/res/res_pjsip/location.c 433794 branches/13/res/res_pjsip.c 433794 branches/13/include/asterisk/res_pjsip.h 433794 Diff: https://reviewboard.asterisk.org/r/4572/diff/ Testing --- All existing pjsip related tests complete. I'll also have to add event generation on contact status change and the tests based on it. Thanks, George Joseph -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4572: RFC: Refactor Qualify and res_pjsip/endpt_send_request
On April 1, 2015, 6:21 p.m., Mark Michelson wrote: As a concept, this feels like a suitable stop-gap solution. Better solutions would be alterations to PJSIP itself. Either 1) Make pjsip_endpt_send_request() not ignore the timeout parameter passed to it. 2) Make pjsip transaction timers configurable per transaction. The solution provided here has the issue that it has an Asterisk scheduler and a PJLIB timer competing with each other, when it would be less wasteful to have the PJLIB timer do everything. Because of the competing timers, it's possible for the transaction state callback to be called into from multiple threads at the same time. If the transaction timers could be altered per transaction, then there would only ever be a single timer running, and the possibility of races is eliminated. I'm going to work on a pjproject patch as well but it'll probably take a while to get mainlined. The sched timer itself doesn't run the callback when it triggers, it only cancels the transaction and pjsip runs the callback so the callback should never be called by multiple threads. - George --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/#review15026 --- On March 30, 2015, 11:57 p.m., George Joseph wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/ --- (Updated March 30, 2015, 11:57 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24863 https://issues.asterisk.org/jira/browse/ASTERISK-24863 Repository: Asterisk Description --- This review is based on the discussion here http://lists.digium.com/pipermail/asterisk-dev/2015-March/073921.html - Right now when a contact is qualified, it's immediately marked as UNAVAILABLE. If a response to the OPTIONS message comes back, it's marked as AVAILABLE again and the round trip time calculated. The status flapping even in normal operation makes generating events tricky because the subscriber will see up/down events even if the contact is really AVAILABLE. Now the pjsip transaction will eventually time out at an unconfigurable 32 seconds and call the callback but 32 seconds is a long time. This is also an issue if the qualify_frequency is less than 32 seconds since they'll be multiple pjsip transactions in progress for the same contact. snip So, I'm proposing pulling libpjsip/pjsip_endpt_send_request up into res_pjsip. It's 2 short function and a pjsip_module structure with no access to any private pjsip stuff. Now we'll have the ability to terminate the transaction AND it seems that there's a timeout member of pjsip_transaction which I'm hoping (but haven't tested) will eliminate the need to add timeout processing in pjsip_options. The result of all of this is that we'll be able to generate events for contact status (ASTERISK-24863) which in turn will help is provide functionality like marking an endpoint unavailable when all of its contacts are unavailable. - Both Josh and Mark had comments in the thread so this is an RFC ONLY review. It covers the pull up of libpjsip/pjsip_endpt_send_request but I've also added the use case which is creating and processing a new aor/contact option called 'qualify_timeout'. I'll split the final submission into 2 pieces. Diffs - branches/13/res/res_pjsip/pjsip_options.c 433794 branches/13/res/res_pjsip/location.c 433794 branches/13/res/res_pjsip.c 433794 branches/13/include/asterisk/res_pjsip.h 433794 Diff: https://reviewboard.asterisk.org/r/4572/diff/ Testing --- All existing pjsip related tests complete. I'll also have to add event generation on contact status change and the tests based on it. Thanks, George Joseph -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4572: RFC: Refactor Qualify and res_pjsip/endpt_send_request
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/#review15026 --- As a concept, this feels like a suitable stop-gap solution. Better solutions would be alterations to PJSIP itself. Either 1) Make pjsip_endpt_send_request() not ignore the timeout parameter passed to it. 2) Make pjsip transaction timers configurable per transaction. The solution provided here has the issue that it has an Asterisk scheduler and a PJLIB timer competing with each other, when it would be less wasteful to have the PJLIB timer do everything. Because of the competing timers, it's possible for the transaction state callback to be called into from multiple threads at the same time. If the transaction timers could be altered per transaction, then there would only ever be a single timer running, and the possibility of races is eliminated. - Mark Michelson On March 31, 2015, 5:57 a.m., George Joseph wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4572/ --- (Updated March 31, 2015, 5:57 a.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24863 https://issues.asterisk.org/jira/browse/ASTERISK-24863 Repository: Asterisk Description --- This review is based on the discussion here http://lists.digium.com/pipermail/asterisk-dev/2015-March/073921.html - Right now when a contact is qualified, it's immediately marked as UNAVAILABLE. If a response to the OPTIONS message comes back, it's marked as AVAILABLE again and the round trip time calculated. The status flapping even in normal operation makes generating events tricky because the subscriber will see up/down events even if the contact is really AVAILABLE. Now the pjsip transaction will eventually time out at an unconfigurable 32 seconds and call the callback but 32 seconds is a long time. This is also an issue if the qualify_frequency is less than 32 seconds since they'll be multiple pjsip transactions in progress for the same contact. snip So, I'm proposing pulling libpjsip/pjsip_endpt_send_request up into res_pjsip. It's 2 short function and a pjsip_module structure with no access to any private pjsip stuff. Now we'll have the ability to terminate the transaction AND it seems that there's a timeout member of pjsip_transaction which I'm hoping (but haven't tested) will eliminate the need to add timeout processing in pjsip_options. The result of all of this is that we'll be able to generate events for contact status (ASTERISK-24863) which in turn will help is provide functionality like marking an endpoint unavailable when all of its contacts are unavailable. - Both Josh and Mark had comments in the thread so this is an RFC ONLY review. It covers the pull up of libpjsip/pjsip_endpt_send_request but I've also added the use case which is creating and processing a new aor/contact option called 'qualify_timeout'. I'll split the final submission into 2 pieces. Diffs - branches/13/res/res_pjsip/pjsip_options.c 433794 branches/13/res/res_pjsip/location.c 433794 branches/13/res/res_pjsip.c 433794 branches/13/include/asterisk/res_pjsip.h 433794 Diff: https://reviewboard.asterisk.org/r/4572/diff/ Testing --- All existing pjsip related tests complete. I'll also have to add event generation on contact status change and the tests based on it. Thanks, George Joseph -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
