What I was trying to say was, if we keep the thread handles abstract, we can switch from pthreads to stdc++ threads without impacting the plugins.
On Tue, Mar 5, 2019 at 5:02 PM Leif Hedstrom <zw...@apache.org> wrote: > > > > On Mar 5, 2019, at 2:24 PM, Walt Karas <wka...@verizonmedia.com.INVALID> > wrote: > > > > Would we ever want to switch over to using libstdc++ threads? > > > I’m likely ok with that, but we have to figure out the story with > retaining C APIs. I.e. this is a big decision to make, making ts/ts.h > requiring C++ to compile. > > I’m not *opposed* to such a change; however, I feel it needs to be a well > thought through and generally accepted community decision. This ties into > other things, such as how to change TSDebug() to use the bwformat APIs. > > — Leif > > > > > On Tue, Mar 5, 2019 at 3:12 PM Leif Hedstrom <zw...@apache.org> wrote: > > > >> I still think it might be time to investigate if eliminating this > >> abstraction is better. Like I said, we only support posix threads > anyways > >> so this abstraction is just extra overhead I assume ? > >> > >> — Leif > >> > >>> On Mar 4, 2019, at 09:57, Fei Deng <f...@verizonmedia.com.invalid> > >> wrote: > >>> > >>> I don't like setting cancel state and type myself either, but at the > >> time I > >>> couldn't come up with other ways of making sure the thread are in the > >> right > >>> state/type. I think the idea just returning a pthread_t to the plugin > is > >> a > >>> good idea, it can also solve the problem with the set state/type, I > >> didn't > >>> know we are allowed to return that information since we wrapped it so > >> deep. > >>> > >>>> On Thu, Feb 28, 2019 at 11:02 PM Leif Hedstrom <zw...@apache.org> > >> wrote: > >>>> > >>>> Good points. > >>>> > >>>> And, is here even a reason to keep our own thread APIs? We only > support > >>>> posix threads anyways. > >>>> > >>>> — Leif > >>>> > >>>>> On Feb 28, 2019, at 21:46, James Peach <jpe...@apache.org> wrote: > >>>>> > >>>>> > >>>>> > >>>>>> On Feb 28, 2019, at 9:17 AM, Fei Deng <duke8...@apache.org> wrote: > >>>>>> > >>>>>> void TSThreadSetCancelState(TSThread thread, int state); > >>>>>> void TSThreadSetCancelType(TSThread thread, int type); > >>>>> > >>>>> I guess you are supposed to pass PTHREAD_CANCEL_* for type type > >>>> parameter? I don't like how this leaks the pthread implementation. An > >> API > >>>> should be self-contained. you also don't capture the possible EINVAL > >> return > >>>> value here. > >>>>> > >>>>>> void TSThreadCancel(TSThread thread); > >>>>> > >>>>> I'm generally -1 on pthread cancellation since I feel like it is very > >>>> hard to use without leaking resources. Maybe your use case can be > >> solved by > >>>> an in-band notification to the thread that it should exit (e.g. atomic > >>>> variable, massage send, file descriptor signal) that is then > >> synchronized > >>>> on the thread join. > >>>>> > >>>>> Maybe diligent application of pthread_cleanup_push(3) can make > >>>> cancellation more robust, but that's not part of this proposal. > >>>>> > >>>>> FWIW I'd be sympathetic to a general API that just returns the > >>>> underlying pthread_t so you can color outside the lines if needed: > >>>>> > >>>>> void *TSThreadGetPlatformThread((TSThread) > >>>>> > >>>>>> void *TSThreadJoin(TSThread thread); > >>>>> > >>>>> This seems quite reasonable. > >>>>> > >>>>>> > >>>>>> Some plugins have been causing a lot of crashes during ATS shutdown, > >> the > >>>>>> root cause is due plugin threads are not aware of ATS is shutting > down > >>>> and > >>>>>> still trying to do stuff such as initiating ssl handshake. The > >>>> workaround > >>>>>> right now is to set a flag using the newly implemented > >>>>>> `SHUTDOWN_LIFECYCLE_HOOK`, but there will still be race conditions > >> since > >>>>>> some threads have a very long turnaround time. > >>>>>> > >>>>>> These new APIs expose corresponding pthread calls so plugins can > have > >> a > >>>>>> better control of its own threads. > >>>>> > >>>> > >>>> > >> > >> > >
