Yes, that makes sense, there could be a warning in the documentation that the critical section must be minimal as it blocks an entire Event Thread.
>From my reading of the code, TSReenableVConn() can queue the next continuation of the hook, due to a mutex associated with the TLS connection not the continuation. So I'm still not seeing any real value in preventing continuations from being queued for later execution because of their own mutexes. On Tue, Feb 12, 2019 at 11:33 AM Bryan Call <bc...@apache.org> wrote: > > I don’t see a way myself, that is why I was suggesting doing a blocking lock. > If the users passes a mutex then there should be a critical section in the > callback and they would be doing a blocking lock anyways. > > -Bryan > > > > On Feb 12, 2019, at 9:28 AM, Walt Karas <wka...@verizonmedia.com.INVALID> > wrote: > > I don't see any way to change the API to require a TSCont that has no > mutex. Unless we create some new type like TSContNoMutex, but there > would be a ton of cascading consequences that don't seem worth it. > > On Tue, Feb 12, 2019 at 11:25 AM Bryan Call <bc...@apache.org> wrote: > > > I am against creating an API that accepts a mutex and then later asserts > because a mutex was passed. > > This goes against the work that has been done on creating continuations with > a mutex and ATS is supposed to handle the locking for you. > > -Bryan > > > > On Feb 11, 2019, at 8:19 AM, Alan Carroll > <solidwallofc...@verizonmedia.com.INVALID> wrote: > > Putting a blocking lock on those calls will eliminate the advantages we got > from moving to 1.1.1 from 1.0.2 in terms of lock contention. It will mean > people will write plugins with locks, put them on a TLS hook, and then > complain "ATS sucks, it's so slow doing TLS". > > Also, it's already very easy to write code that asserts later on. Trying > scheduling a TSCont that doesn't have a mutex. > > On Fri, Feb 8, 2019 at 7:38 PM Susan Hinrichs > <shinr...@verizonmedia.com.invalid> wrote: > > Yes, we could blocking lock easily enough too at the potential cost of > blocking the thread. > > We could block and issue a warning on the hook call to give people some > hint that this might be a performance problem. > > On Fri, Feb 8, 2019, 7:21 PM Bryan Call <bc...@apache.org wrote: > > Couldn’t you change the lock to be a blocking lock for the SSL APIs? > > It seems like a bad interface to be able to write and compile code that > will assert later on. > > -Bryan > > On Feb 7, 2019, at 2:51 PM, Susan Hinrichs <shinr...@apache.org> > > wrote: > > > @macisasandwich ran into this when working with port ready changes in > autest. The extra port probe tied up the test ssl plugin (for > > tls_hooks15) > > which exercised a TLS hook on a continuation with a mutex. Since the > > invoke > > method could not grab the lock, the assertion went off. > > I went back to look at how the TLS code should grab the continuation > > lock > > before calling invoke. But there are many (most) cases where the TLS > > hook > > cannot be delayed by a reschedule in the case when the lock cannot be > obtained. > > In most cases for such global continuations, you would not want a lock > > on > > the continuation for performance reasons. In the case where locking is > needed, it would be better done by the plugin writer internal to the > > plugin. > > > I made a PR with code changes to force continuations to not have > > mutexes > > if > > they are using the SSL hooks. With this code change, Trafficserver > > will > > assert if a continuation with a mutex tries to attach to a SSL hooks. > > The PR with code change > > https://github.com/apache/trafficserver/pull/4939 > > > Please share your comments/concerns. > > Thanks, > Susan > > > > > >
