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. > > > >
