You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by Sudheer Vinukonda <su...@yahoo.com.INVALID> on 2020/05/18 23:38:38 UTC
CPP API change - TXN_CLOSE event handling in TransactionPlugin
We recently ran into a core dump issue due to one of our TransactionPlugin's continuations being called back with TXN_CLOSE event.
The current CPPAPI's TransactionPlugin does not expect a call back on TXN_CLOSE, and asserts on it. This is because the expectation seems to be that all the continuations for TransactionPlugins attached to the Transaction are destroyed in the global TXN_CLOSE hook. However, this only works based on the plugin table in the Transaction object, which gets populated based on `Transaction::addPlugin()`. Turns out, some of our plugins do not call `Transaction::addPlugin()` and just create a `TransactionPlugin()` and register hooks to it resulting in them getting called back for TXN_CLOSE event as well.
Long story short, it didn't particularly seem unreasonable to add support to TXN_CLOSE event in TransactionPlugin, since the current API does not seem to mandate calling `Transaction.addPlugin()`.
https://github.com/apache/trafficserver/pull/6800
Please let me know if there are any concerns/comments.
Re: CPP API change - TXN_CLOSE event handling in TransactionPlugin
Posted by Sudheer Vinukonda <su...@yahoo.com.INVALID>.
Yeah, We’ve plugins that are in mixed mode and add txn-close hook through C API.
If the plugin is just using CPP API, then yes you are right - it’d not crash but leak.
> On May 18, 2020, at 6:15 PM, Walt Karas <wk...@verizonmedia.com.invalid> wrote:
>
> Hmm I think if you forgot to call 'Transaction::addPlugin()' you would just
> leak the plugin object, I don't think it would cause the crash you saw.
>
>> On Mon, May 18, 2020 at 6:38 PM Sudheer Vinukonda
>> <su...@yahoo.com.invalid> wrote:
>>
>> We recently ran into a core dump issue due to one of our
>> TransactionPlugin's continuations being called back with TXN_CLOSE event.
>> The current CPPAPI's TransactionPlugin does not expect a call back on
>> TXN_CLOSE, and asserts on it. This is because the expectation seems to be
>> that all the continuations for TransactionPlugins attached to the
>> Transaction are destroyed in the global TXN_CLOSE hook. However, this only
>> works based on the plugin table in the Transaction object, which gets
>> populated based on `Transaction::addPlugin()`. Turns out, some of our
>> plugins do not call `Transaction::addPlugin()` and just create a
>> `TransactionPlugin()` and register hooks to it resulting in them getting
>> called back for TXN_CLOSE event as well.
>> Long story short, it didn't particularly seem unreasonable to add support
>> to TXN_CLOSE event in TransactionPlugin, since the current API does not
>> seem to mandate calling `Transaction.addPlugin()`.
>>
>> https://github.com/apache/trafficserver/pull/6800
>>
>> Please let me know if there are any concerns/comments.
Re: CPP API change - TXN_CLOSE event handling in TransactionPlugin
Posted by Walt Karas <wk...@verizonmedia.com.INVALID>.
Hmm I think if you forgot to call 'Transaction::addPlugin()' you would just
leak the plugin object, I don't think it would cause the crash you saw.
On Mon, May 18, 2020 at 6:38 PM Sudheer Vinukonda
<su...@yahoo.com.invalid> wrote:
> We recently ran into a core dump issue due to one of our
> TransactionPlugin's continuations being called back with TXN_CLOSE event.
> The current CPPAPI's TransactionPlugin does not expect a call back on
> TXN_CLOSE, and asserts on it. This is because the expectation seems to be
> that all the continuations for TransactionPlugins attached to the
> Transaction are destroyed in the global TXN_CLOSE hook. However, this only
> works based on the plugin table in the Transaction object, which gets
> populated based on `Transaction::addPlugin()`. Turns out, some of our
> plugins do not call `Transaction::addPlugin()` and just create a
> `TransactionPlugin()` and register hooks to it resulting in them getting
> called back for TXN_CLOSE event as well.
> Long story short, it didn't particularly seem unreasonable to add support
> to TXN_CLOSE event in TransactionPlugin, since the current API does not
> seem to mandate calling `Transaction.addPlugin()`.
>
> https://github.com/apache/trafficserver/pull/6800
>
> Please let me know if there are any concerns/comments.