When using TSContSchedule() and TSContScheduleAPI() set the calling thread as the thread affinity when not already set

[sudheerv]

It is unreasonable to expect TSContSchedule() and TSContScheduleAPI() to explicitly call TSContSetThreadAffinity() to work. The API should automatically handle the case when the thread affinity has not been set already.

This not only is an incompatible change (which I admit is not the unreasonable part given that this is a major release), but, forcing the user to call another API, instead of implicitly handling it doesn't sound reasonable.

Email to dev@

Walt Karas <wkaras@verizonmedia.com.invalid>
To:
dev
Cc:
ljindal@linkedin.com
,
Saurav Kumar

Thu, Mar 26 at 12:51 PM

I was having problems with this too in a new plugin I was writing.  I had
to make this small change to get it to work:
https://github.com/apache/trafficserver/pull/6489/files#diff-8c09c39a3b13e183ba1bfd1e590cfb21

I found that I had to use TSContScheduleOnPool() instead of
TSContSchedule().

On Thu, Mar 26, 2020 at 12:55 PM Sudheer Vinukonda
<sudheervinukonda@yahoo.com.invalid> wrote:

> While investigating a core dump that we ran into during our ATS9
> migration, we discovered a couple of problems with the thread affinity
> changes and the TSContSchedule API.
> 1) It looks like the API TSContSchedule() and TSContScheduleEvery() now
> don't work unless TSContSetThreadAffinity() was called prior to calling
> them. This sounds unreasonable, so, I'm proposing to change the API to set
> the Continuation's thread affinity to the calling thread of the API, if not
> already set.
> 2) We have a use case where one of our plugins calls TSContSchedule* API
> from a non-ATS thread (created using pthread_create). This borks in a weird
> fashion because the FORCE_PLUGIN_SCOPED_MUTEX ends up not acquiring the
> mutex (because the calling thread isn't an EThread, so, this_ethread() is
> NULL and the holding thread for the cont's mutex is also NULL, which means
> in Mutex_Lock(), the if (m->thread_holding != t) { evaluates to false
> bypassing the mutex acquire call. However, the subsequent mutex release
> does get called via Mutex_unlock() and this makes bad things to happen (as
> the __nusers of the mutex object goes below 0 (it's an unsigned int)).
>
> https://github.com/apache/trafficserver/blob/4a503c817766fcad3b9eab3ff3a8e9e73d483ae7/iocore/eventsystem/I_Lock.h#L320
>
> So, we could either
> - prevent calling TSContSchedule*() API from non-ATS threads by asserting
> if this_ethread() is NULL before trying to acquire the mutex. ie.
> basically, fail hard and fast and in more obvious way (than an obscure
> random side-effect that happens much later because a release() was called
> on a lock without calling an acquire())
> OR
> - With the thread affinity changes, we could check if the continuation has
> a thread affinity set to an EThread and still allow calling
> TSContSchedule*() API from non-ATS Threads, as long as the thread affinity
> is set to an ATS thread. This requires to acquire the mutex on the affinity
> thread than the calling thread.
>
> After discussing with Bryan, we prefer option (1) which is consistent with
> the expectation that Continuation Schedule API should only be called from
> ATS threads.
>
> Note: Both the proposed changes here should not be breaking any
> compatibility or expectations, but rather retain compatibility with (1)
> above and make the API a bit more predictable/robust with (2)
>
> Let me know if there are any comments/concerns or questions.
>
> Thanks,
> Sudheer
>

[duke8253]

I'm not sure this is a good idea. The point of making the API changes is to make sure when writing new plugins, people need to understand there is a change to how threads are been used, and it is deliberately set to have it return null when calling the original api if no affinity thread is set. And even if you wish to set an affinity thread, I don't think using the "this_thread" is a good idea, since you might just end up on a thread you don't want. If you're experiencing errors when migrating, my suggestion is to just change all the calls to TSContSchedule to TSContScheduleOnPool, which accepts the same arguments and functions mostly the same, or we can rename TSContScheduleOnPool to be TSContSchedule again and remove the current TSContSchedule. The underlying logic of picking which thread is being used should be able to handle it.

[sudheerv]

I'm not sure this is a good idea. The point of making the API changes is to make sure when writing new plugins, people need to understand there is a change to how threads are been used, and it is deliberately set to have it return null when calling the original api if no affinity thread is set. And even if you wish to set an affinity thread, I don't think using the "this_thread" is a good idea, since you might just end up on a thread you don't want. If you're experiencing errors when migrating, my suggestion is to just change all the calls to TSContSchedule to TSContScheduleOnPool, which accepts the same arguments and functions mostly the same, or we can rename TSContScheduleOnPool to be TSContSchedule again and remove the current TSContSchedule. The underlying logic of picking which thread is being used should be able to handle it.

I don't agree that we expect another API to be called to just to use TSContSchedule. As a plugin user, I should be able to create a CONT and schedule it without bothering about affinity unless I've to. Otherwise, it seems poor both as an interface and intuitively. I think it should be acceptable to document that without setting Affinity explicitly, the calling thread's affinity is used (I'll add the doc update separately as I've another PR where I changed the doc). Silently returning null when no affinity is set, makes the API rather unusable IMO and we obviously do not want to also assert for this (it'd be rather silly to assert for calling TSContSchedule).

[duke8253]

I'm not sure this is a good idea. The point of making the API changes is to make sure when writing new plugins, people need to understand there is a change to how threads are been used, and it is deliberately set to have it return null when calling the original api if no affinity thread is set. And even if you wish to set an affinity thread, I don't think using the "this_thread" is a good idea, since you might just end up on a thread you don't want. If you're experiencing errors when migrating, my suggestion is to just change all the calls to TSContSchedule to TSContScheduleOnPool, which accepts the same arguments and functions mostly the same, or we can rename TSContScheduleOnPool to be TSContSchedule again and remove the current TSContSchedule. The underlying logic of picking which thread is being used should be able to handle it.

I don't agree that we expect another API to be called to just to use TSContSchedule. As a plugin user, I should be able to create a CONT and schedule it without bothering about affinity unless I've to. Otherwise, it seems poor both as an interface and intuitively. I think it should be acceptable to document that without setting Affinity explicitly, the calling thread's affinity is used (I'll add the doc update separately as I've another PR where I changed the doc). Silently returning null when no affinity is set, makes the API rather unusable IMO.

That's why i suggest to remove that API completely and rename TSContScheduleOnPool to TSContSchedule. Because if you do the change you are making right now, any continuation you schedule from a plugin will just end up on a task thread, and the same task thread.

[sudheerv]

I'm not sure this is a good idea. The point of making the API changes is to make sure when writing new plugins, people need to understand there is a change to how threads are been used, and it is deliberately set to have it return null when calling the original api if no affinity thread is set. And even if you wish to set an affinity thread, I don't think using the "this_thread" is a good idea, since you might just end up on a thread you don't want. If you're experiencing errors when migrating, my suggestion is to just change all the calls to TSContSchedule to TSContScheduleOnPool, which accepts the same arguments and functions mostly the same, or we can rename TSContScheduleOnPool to be TSContSchedule again and remove the current TSContSchedule. The underlying logic of picking which thread is being used should be able to handle it.

I don't agree that we expect another API to be called to just to use TSContSchedule. As a plugin user, I should be able to create a CONT and schedule it without bothering about affinity unless I've to. Otherwise, it seems poor both as an interface and intuitively. I think it should be acceptable to document that without setting Affinity explicitly, the calling thread's affinity is used (I'll add the doc update separately as I've another PR where I changed the doc). Silently returning null when no affinity is set, makes the API rather unusable IMO.

That's why i suggest to remove that API completely and rename TSContScheduleOnPool to TSContSchedule. Because if you do the change you are making right now, any thread you schedule from a plugin will just end up on a task thread.

That should be totally fine IMO, as long as it's documented. Also, not always that a plugin calls TSContSchedule from a task thread. Plugins also run in Net threads when executing hooks. In fact, plugins switching to TASK threads is rather rare and few in number. And in those cases, the plugin user should know if they want to schedule a CONT on net thread and set the affinity accordingly.

[duke8253]

I'm not sure this is a good idea. The point of making the API changes is to make sure when writing new plugins, people need to understand there is a change to how threads are been used, and it is deliberately set to have it return null when calling the original api if no affinity thread is set. And even if you wish to set an affinity thread, I don't think using the "this_thread" is a good idea, since you might just end up on a thread you don't want. If you're experiencing errors when migrating, my suggestion is to just change all the calls to TSContSchedule to TSContScheduleOnPool, which accepts the same arguments and functions mostly the same, or we can rename TSContScheduleOnPool to be TSContSchedule again and remove the current TSContSchedule. The underlying logic of picking which thread is being used should be able to handle it.

I don't agree that we expect another API to be called to just to use TSContSchedule. As a plugin user, I should be able to create a CONT and schedule it without bothering about affinity unless I've to. Otherwise, it seems poor both as an interface and intuitively. I think it should be acceptable to document that without setting Affinity explicitly, the calling thread's affinity is used (I'll add the doc update separately as I've another PR where I changed the doc). Silently returning null when no affinity is set, makes the API rather unusable IMO.

That's why i suggest to remove that API completely and rename TSContScheduleOnPool to TSContSchedule. Because if you do the change you are making right now, any thread you schedule from a plugin will just end up on a task thread.

That should be totally fine IMO, as long as it's documented. Also, not always that a plugin calls TSContSchedule from a task thread. Plugins also run in Net threads when executing hooks.

Well, if you only call TSContSchedule with the current change from the plugin it will, since you never jumped to a net thread in the first place.

[duke8253]

Let's see what @SolidWallOfCode have to say on this problem too, since he made a few important decisions on the original design.

[sudheerv]

I'm not sure this is a good idea. The point of making the API changes is to make sure when writing new plugins, people need to understand there is a change to how threads are been used, and it is deliberately set to have it return null when calling the original api if no affinity thread is set. And even if you wish to set an affinity thread, I don't think using the "this_thread" is a good idea, since you might just end up on a thread you don't want. If you're experiencing errors when migrating, my suggestion is to just change all the calls to TSContSchedule to TSContScheduleOnPool, which accepts the same arguments and functions mostly the same, or we can rename TSContScheduleOnPool to be TSContSchedule again and remove the current TSContSchedule. The underlying logic of picking which thread is being used should be able to handle it.

I don't agree that we expect another API to be called to just to use TSContSchedule. As a plugin user, I should be able to create a CONT and schedule it without bothering about affinity unless I've to. Otherwise, it seems poor both as an interface and intuitively. I think it should be acceptable to document that without setting Affinity explicitly, the calling thread's affinity is used (I'll add the doc update separately as I've another PR where I changed the doc). Silently returning null when no affinity is set, makes the API rather unusable IMO.

That's why i suggest to remove that API completely and rename TSContScheduleOnPool to TSContSchedule. Because if you do the change you are making right now, any thread you schedule from a plugin will just end up on a task thread.

That should be totally fine IMO, as long as it's documented. Also, not always that a plugin calls TSContSchedule from a task thread. Plugins also run in Net threads when executing hooks.

Well, if you only call TSContSchedule with the current change from the plugin it will, since you never jumped to a net thread in the first place.

I don't think so. The plugin call backs run on the same NET threads as the HttpSM. You'd have to switch to a task thread explicitly if you want to.

See the traceback below

19   Thread 0x7fffeae05700 (LWP 286250) "[ET_NET 16]" CookiePolicyEnforcement::CookiePolicyEnforcementPlugin::handleReadRequestHeaders (this=0x7ffff037f1a0, transaction=...) at CookiePolicyEnforcementPlugin.cc:138


(gdb) bt                                     
#0  CookiePolicyEnforcement::CookiePolicyEnforcementPlugin::handleReadRequestHeaders (this=0x7ffff037f1a0, transaction=...) at CookiePolicyEnforcementPlugin.cc:138
#1  0x00007fffe71d1ba7 in (anonymous namespace)::handleGlobalPluginEvents (cont=<optimized out>, event=TS_EVENT_HTTP_READ_REQUEST_HDR, edata=0x7fffec695bc0) at GlobalPlugin.cc:65
#2  0x00000000004d4d91 in INKContInternal::handle_event (this=0x7fffefff8b00, event=60002, edata=0x7fffec695bc0) at traffic_server/InkAPI.cc:1101
#3  0x00000000004e6d0b in handleEvent (data=0x7fffec695bc0, event=60002, this=0x7fffefff8b00)
    at /export/content/data/multiproduct-post-commit/i001/workspace/ats-core_70883cbaaedc2953e9667b350f6aacc22aa09209/ats9/src/iocore/eventsystem/I_Continuation.h:190
#4  APIHook::invoke (this=this@entry=0x7ffff36a3ee0, event=60002, edata=edata@entry=0x7fffec695bc0) at traffic_server/InkAPI.cc:1338
#5  0x00000000005498df in HttpSM::state_api_callout (this=0x7fffec695bc0, event=<optimized out>, data=<optimized out>) at HttpSM.cc:1499
#6  0x00000000005535a3 in HttpSM::set_next_state (this=0x7fffec695bc0) at HttpSM.cc:7385
#7  0x000000000053d69f in HttpSM::call_transact_and_set_next_state (this=this@entry=0x7fffec695bc0, f=<optimized out>) at HttpSM.cc:7351
#8  0x000000000054361a in HttpSM::state_read_client_request_header (this=0x7fffec695bc0, event=<optimized out>, data=<optimized out>) at HttpSM.cc:847
#9  0x000000000054c8e8 in HttpSM::main_handler (this=0x7fffec695bc0, event=100, data=0x7fffebac7fb8) at HttpSM.cc:2729
#10 0x000000000054e808 in HttpSM::handle_api_return (this=0x7fffec695bc0) at HttpSM.cc:1612
#11 0x0000000000549dd6 in HttpSM::state_api_callout (this=0x7fffec695bc0, event=<optimized out>, data=<optimized out>) at HttpSM.cc:1566
#12 0x000000000054c08b in HttpSM::state_add_to_list (this=this@entry=0x7fffec695bc0, event=event@entry=0) at HttpSM.cc:396
#13 0x000000000054dad0 in HttpSM::attach_client_session (this=0x7fffec695bc0, client_vc=client_vc@entry=0x7fffebaaaae8, buffer_reader=0x7ffff1f33a08) at HttpSM.cc:569
#14 0x000000000070e2bd in ProxyTransaction::new_transaction (this=this@entry=0x7fffebaaaae8, from_early_data=<optimized out>) at ProxyTransaction.cc:53
#15 0x000000000051e9ae in new_transaction (this=0x7fffebaaa740) at Http1ClientSession.cc:478
#16 release (trans=0x7fffebaaaae8, this=0x7fffebaaa740) at Http1ClientSession.cc:443
#17 Http1ClientSession::start (this=0x7fffebaaa740) at Http1ClientSession.cc:537
#18 0x000000000070d57b in handle_api_return (event=60000, this=0x7fffebaaa740) at ProxySession.cc:167
#19 ProxySession::do_api_callout (this=this@entry=0x7fffebaaa740, id=id@entry=TS_HTTP_SSN_START_HOOK) at ProxySession.cc:148
#20 0x000000000051f195 in Http1ClientSession::new_connection (this=0x7fffebaaa740, new_vc=<optimized out>, iobuf=<optimized out>, reader=<optimized out>) at Http1ClientSession.cc:205
#21 0x0000000000516705 in HttpSessionAccept::accept (this=0x7ffff0346200, netvc=<optimized out>, iobuf=<optimized out>, reader=0x7ffff1f33a08) at HttpSessionAccept.cc:57
#22 0x000000000070bf39 in ProtocolProbeTrampoline::ioCompletionEvent (this=0x7fffea0df080, event=<optimized out>, edata=<optimized out>) at ProtocolProbeSessionAccept.cc:147
#23 0x000000000075d515 in handleEvent (data=0x7fffebac7fb8, event=100, this=0x7fffea0df080) at /export/content/data/multiproduct-post-commit/i001/workspace/ats-core_70883cbaaedc2953e9667b350f6aacc22aa09209/ats9/src/iocore/eventsystem/I_Continuation.h:190
#24 read_signal_and_update (event=100, vc=0x7fffebac7de0) at UnixNetVConnection.cc:83
#25 0x0000000000761b26 in read_from_net (nh=0x7ffff3116d30, vc=0x7fffebac7de0, thread=0x7ffff3113000) at UnixNetVConnection.cc:312
#26 0x000000000074e594 in NetHandler::process_ready_list (this=this@entry=0x7ffff3116d30) at UnixNet.cc:400
#27 0x000000000074e87d in NetHandler::waitForActivity (this=0x7ffff3116d30, timeout=<optimized out>) at UnixNet.cc:535
#28 0x00000000007a95d1 in EThread::execute_regular (this=this@entry=0x7ffff3113000) at UnixEThread.cc:284
#29 0x00000000007a98a2 in EThread::execute (this=0x7ffff3113000) at UnixEThread.cc:345
#30 0x00000000007a7be9 in spawn_thread_internal (a=0x7ffff36fd480) at Thread.cc:92
#31 0x00007ffff5257dd5 in start_thread () from /lib64/libpthread.so.0
#32 0x00007ffff44e1b3d in clone () from /lib64/libc.so.6
(gdb) 

[SolidWallOfCode]

@sudheerv is correct, plugin hooks are called from ET_NET threads, in fact the same thread as the transaction. Running on an ET_TASK thread requires explicitly specifying that pool.

It comes down to whether we want to have a simpler API or a clearer one. Removing TSContSchedule means every schedule continuation will be assigned to an explicit thread or thread pool. I'm trying to remember if that's what we agreed on back at the summit (where there was a long discussion on this topic).

[sudheerv]

@sudheerv is correct, plugin hooks are called from ET_NET threads, in fact the same thread as the transaction. Running on an ET_TASK thread requires explicitly specifying that pool.

It comes down to whether we want to have a simpler API or a clearer one. Removing TSContSchedule means every schedule continuation will be assigned to an explicit thread or thread pool. I'm trying to remember if that's what we agreed on back at the summit (where there was a long discussion on this topic).

+1

Since, TSContSchedule already exists, so my thinking is that defaulting to the current thread when no explicit affinity is set (with documentation of course), seems to be reasonable behavior. The alternative of silently failing by returning null when no affinity is set seems very undesirable and unintuitive IMHO.

[SolidWallOfCode]

I must have missed this in the PRs, I think the current behavior is not good. My suggestion would be

• Accept this PR.
• Replace TSContSchedule in ATS 10 with TSConstScheduleOnPool, the net effect being the removal of the current TSContSchedule.

I think it's too late to do this for 9, as we should have. Overall I agree with @duke8253 - I just think the timing isn't right to fully change, but we should certainly do that for ATS 10. After pondering I think have an explicit pool or thread will be worth it to improve overall use of scheduled continuations.

[sudheerv]

I must have missed this in the PRs, I think the current behavior is not good. My suggestion would be

• Accept this PR.
• Replace TSContSchedule in ATS 10 with TSConstScheduleOnPool, the net effect being the removal of the current TSContSchedule.

I think it's too late to do this for 9, as we should have. Overall I agree with @duke8253 - I just think the timing isn't right to fully change, but we should certainly do that for ATS 10. After pondering I think have an explicit pool or thread will be worth it to improve overall use of scheduled continuations.

Yeah, I think that sounds reasonable. Thanks Alan!

The only minor concern I'd have is replacing TSContSchedule with TSConstScheduleOnPool. I understand TSConstScheduleOnPool does achieve what TSConstSchedule can do, but the naming is slightly misleading especially, when a user is trying to schedule a CONT on a specific thread (e.g the calling thread). So, while I agree we can dump the existing TSConstSchedule in ATS 10, like @duke8253 recommends, may be we should rename TSConstScheduleOnPool to TSConstSchedule to make it more easier to map the behavior. Of course, this can and should be discussed separately outside this PR.

Thanks for the feedback and thanks in advance for approving it ;)

[SolidWallOfCode]

Allright, we'll just get rid of TSContSchedule and not rename.

[zwoop]

Cherry-picked to v9.0.x branch.

[zwoop]

Allright, we'll just get rid of TSContSchedule and not rename.

This implies there's another PR coming, but that's only for 10.0 ?

[duke8253]

Allright, we'll just get rid of TSContSchedule and not rename.

This implies there's another PR coming, but that's only for 10.0 ?

Yep