You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by "Chou, Peter" <pb...@labs.att.com> on 2017/06/21 22:22:53 UTC
Help with collapsed_connection plugin.
Hi,
I am taking a look at the collapsed_connection plugin. There appears to be an assertion failure crash incompatibility between this plugin and changes made in the 6.2.x branch. TS-4387 appears to require all continuations calling TSContSchedule() to have a mutex, but in the plugin no mutex is created in addMutexRetry() since TSContCreate() is called as TSContCreate(retryCacheUrlLock, NULL).
Is it sufficient to change this call to TSContCreate(retryCacheUrlLock, TSMutexCreate())?
Would this possibly cause side-effects such as mutex lock contention the plugin is not designed to deal with, and would the mutex need to be destroyed manually somewhere else in the code? I am still learning on this area of ATS...
Thanks,
Peter
Re: Help with collapsed_connection plugin.
Posted by James Peach <jp...@apache.org>.
> On Jun 23, 2017, at 12:44 AM, John Rushford <jj...@gmail.com> wrote:
>
> Leif,
>
> The mutex is ref counted but the i look at the code for TSContDestroy, I
> see that the pointer to the mutex is only set to NULL,
> INKContInternal::destroy(). I think that the plugins should call
> TSMutexDestroy() also. Is that correct?
Plugins should *never* call TSMutexDestroy() once the core has refcounted it. TSMutexDestroy() immediately deallocates the mutex without examining the refcount. There’s API for plugins to refcount a TSMutex.
The correct pattern is as Peter originally suggests:
TSContCreate(retryCacheUrlLock, TSMutexCreate());
>
> On Thu, Jun 22, 2017 at 6:59 AM, Leif Hedstrom <zw...@apache.org> wrote:
>
>> Yes, this is the correct fix. In earlier versions, the core would create
>> the mutex for you if it needed one, but this had some pretty expensive
>> overhead. So now we just assert instead, so people can fix their plugins
>> (and our plugins apparently).
>>
>> Fwiw, the mutex is needed when the continuation uses a specific set of
>> APIs. This should be documented in the docs somewhere.
>>
>> Cheers,
>>
>> -- Leif
>>
>>> On Jun 21, 2017, at 8:18 PM, John Rushford <jj...@gmail.com> wrote:
>>>
>>>
>>> Peter,
>>>
>>> I've found this with a couple of other plugins and fixed them by making
>> exactly the change you you're asking about. In testing i have not run into
>> any issues with the plugins i have changed.
>>>
>>> John
>>>
>>>> On Jun 21, 2017, at 4:22 PM, Chou, Peter <pb...@labs.att.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I am taking a look at the collapsed_connection plugin. There appears to
>> be an assertion failure crash incompatibility between this plugin and
>> changes made in the 6.2.x branch. TS-4387 appears to require all
>> continuations calling TSContSchedule() to have a mutex, but in the plugin
>> no mutex is created in addMutexRetry() since TSContCreate() is called as
>> TSContCreate(retryCacheUrlLock, NULL).
>>>>
>>>> Is it sufficient to change this call to TSContCreate(retryCacheUrlLock,
>> TSMutexCreate())?
>>>>
>>>> Would this possibly cause side-effects such as mutex lock contention
>> the plugin is not designed to deal with, and would the mutex need to be
>> destroyed manually somewhere else in the code? I am still learning on this
>> area of ATS...
>>>>
>>>> Thanks,
>>>> Peter
>>
>>
>
>
> --
> John Rushford
> jjrushford@gmail.com
Re: Help with collapsed_connection plugin.
Posted by John Rushford <jj...@gmail.com>.
Leif,
The mutex is ref counted but the i look at the code for TSContDestroy, I
see that the pointer to the mutex is only set to NULL,
INKContInternal::destroy(). I think that the plugins should call
TSMutexDestroy() also. Is that correct?
On Thu, Jun 22, 2017 at 6:59 AM, Leif Hedstrom <zw...@apache.org> wrote:
> Yes, this is the correct fix. In earlier versions, the core would create
> the mutex for you if it needed one, but this had some pretty expensive
> overhead. So now we just assert instead, so people can fix their plugins
> (and our plugins apparently).
>
> Fwiw, the mutex is needed when the continuation uses a specific set of
> APIs. This should be documented in the docs somewhere.
>
> Cheers,
>
> -- Leif
>
> > On Jun 21, 2017, at 8:18 PM, John Rushford <jj...@gmail.com> wrote:
> >
> >
> > Peter,
> >
> > I've found this with a couple of other plugins and fixed them by making
> exactly the change you you're asking about. In testing i have not run into
> any issues with the plugins i have changed.
> >
> > John
> >
> >> On Jun 21, 2017, at 4:22 PM, Chou, Peter <pb...@labs.att.com> wrote:
> >>
> >> Hi,
> >>
> >> I am taking a look at the collapsed_connection plugin. There appears to
> be an assertion failure crash incompatibility between this plugin and
> changes made in the 6.2.x branch. TS-4387 appears to require all
> continuations calling TSContSchedule() to have a mutex, but in the plugin
> no mutex is created in addMutexRetry() since TSContCreate() is called as
> TSContCreate(retryCacheUrlLock, NULL).
> >>
> >> Is it sufficient to change this call to TSContCreate(retryCacheUrlLock,
> TSMutexCreate())?
> >>
> >> Would this possibly cause side-effects such as mutex lock contention
> the plugin is not designed to deal with, and would the mutex need to be
> destroyed manually somewhere else in the code? I am still learning on this
> area of ATS...
> >>
> >> Thanks,
> >> Peter
>
>
--
John Rushford
jjrushford@gmail.com
Re: Help with collapsed_connection plugin.
Posted by Leif Hedstrom <zw...@apache.org>.
Yes, this is the correct fix. In earlier versions, the core would create the mutex for you if it needed one, but this had some pretty expensive overhead. So now we just assert instead, so people can fix their plugins (and our plugins apparently).
Fwiw, the mutex is needed when the continuation uses a specific set of APIs. This should be documented in the docs somewhere.
Cheers,
-- Leif
> On Jun 21, 2017, at 8:18 PM, John Rushford <jj...@gmail.com> wrote:
>
>
> Peter,
>
> I've found this with a couple of other plugins and fixed them by making exactly the change you you're asking about. In testing i have not run into any issues with the plugins i have changed.
>
> John
>
>> On Jun 21, 2017, at 4:22 PM, Chou, Peter <pb...@labs.att.com> wrote:
>>
>> Hi,
>>
>> I am taking a look at the collapsed_connection plugin. There appears to be an assertion failure crash incompatibility between this plugin and changes made in the 6.2.x branch. TS-4387 appears to require all continuations calling TSContSchedule() to have a mutex, but in the plugin no mutex is created in addMutexRetry() since TSContCreate() is called as TSContCreate(retryCacheUrlLock, NULL).
>>
>> Is it sufficient to change this call to TSContCreate(retryCacheUrlLock, TSMutexCreate())?
>>
>> Would this possibly cause side-effects such as mutex lock contention the plugin is not designed to deal with, and would the mutex need to be destroyed manually somewhere else in the code? I am still learning on this area of ATS...
>>
>> Thanks,
>> Peter
Re: Help with collapsed_connection plugin.
Posted by John Rushford <jj...@gmail.com>.
Peter,
I've found this with a couple of other plugins and fixed them by making exactly the change you you're asking about. In testing i have not run into any issues with the plugins i have changed.
John
> On Jun 21, 2017, at 4:22 PM, Chou, Peter <pb...@labs.att.com> wrote:
>
> Hi,
>
> I am taking a look at the collapsed_connection plugin. There appears to be an assertion failure crash incompatibility between this plugin and changes made in the 6.2.x branch. TS-4387 appears to require all continuations calling TSContSchedule() to have a mutex, but in the plugin no mutex is created in addMutexRetry() since TSContCreate() is called as TSContCreate(retryCacheUrlLock, NULL).
>
> Is it sufficient to change this call to TSContCreate(retryCacheUrlLock, TSMutexCreate())?
>
> Would this possibly cause side-effects such as mutex lock contention the plugin is not designed to deal with, and would the mutex need to be destroyed manually somewhere else in the code? I am still learning on this area of ATS...
>
> Thanks,
> Peter