Skip to content

Conversation

@scw00
Copy link
Member

@scw00 scw00 commented Jan 24, 2019

The plugin might schedule the sm to task thread. And it will crash here because task thread doses not have thread session pool.

static int
task_cont_handler(TSCont contp, TSEvent event, void *edata)
{
  TSHttpTxn txn = (TSHttpTxn)TSContDataGet(contp);
  TSHttpTxnReenable(txn, TS_EVENT_HTTP_CONTINUE);
  TSContDestroy(contp);
  return 0;
}

static int
cont_handler(TSCont contp, TSEvent event, void *edata)
{

  auto cont = TSContCreate(task_cont_handler, TSMutexCreate());
  TSContDataSet(cont, edata);
  TSContScheduleOnPool(cont, 0, TS_THREAD_POOL_TASK);
  return 0;
}

void
TSPluginInit(int argc, const char *argv[])
{
  TSPluginRegistrationInfo info;

  info.plugin_name   = PLUGIN_NAME;
  info.vendor_name   = "Apache Software Foundation";
  info.support_email = "dev@trafficserver.apache.org";

  if (TSPluginRegister(&info) != TS_SUCCESS) {
    TSError("[%s] Plugin registration failed", PLUGIN_NAME);
  }

  TSDebug(PLUGIN_NAME, "Hello World!");

  TSHttpHookAdd(TS_HTTP_SEND_RESPONSE_HDR_HOOK, TSContCreate(cont_handler, TSMutexCreate()));
}

@scw00 scw00 self-assigned this Jan 24, 2019
@scw00 scw00 added the HTTP label Jan 24, 2019
@scw00 scw00 added this to the 9.0.0 milestone Jan 24, 2019
@scw00 scw00 force-pushed the none_server_session branch from e549515 to f40fed8 Compare January 24, 2019 09:44
@scw00 scw00 changed the title Closes Server Session when local session pool is nullptr Assert when sm callback to wrong thread Jan 24, 2019
@scw00
Copy link
Member Author

scw00 commented Jan 24, 2019

Updated for @oknet's suggestion. We limit the thread when we callback. I have no idea which one would be better .

shinrich
shinrich previously approved these changes Jan 24, 2019
@shinrich
Copy link
Member

Alternatively should the InkAPI call just reschedule to an ET NET thread if it lands on the wrong type fo thread?

@bryancall
Copy link
Contributor

I would rather have the plugin assert instead of having a bug in the plugin and it going unnoticed because we are fixing things up under the hood.

bryancall
bryancall previously approved these changes Jan 24, 2019
@zwoop
Copy link
Contributor

zwoop commented Jan 24, 2019

Agreed. @shinrich How does this relate to the other changes that were done with regards to locks (mutexes) and automatically acquire those in many cases ?

@scw00 scw00 dismissed stale reviews from bryancall and shinrich via 0636a44 January 25, 2019 03:57
@scw00 scw00 force-pushed the none_server_session branch from f40fed8 to 0636a44 Compare January 25, 2019 03:57
@oknet
Copy link
Member

oknet commented Jan 25, 2019

Agreed with @shinrich , the TSHttpSMCallback is designed to reenable HttpSM from the correct EThread.

By TS-2271, TSHttpTxnReenable() can be called from a thread which was not created using the ATS EThread API.

Now we can also allow TSHttpTxnReenable() to be called from an EThread outside of ET_NET.

@shinrich
Copy link
Member

I think this is unrelated to the auto-locking concerns. This code is already handling the reschedule. The original code would reschedule for any non REGULAR thread. The changes will now reschedule for any non REGULAR or non ET_NET threads.

So the case that will now be rescheduled that wouldn't have been in the original would be REGULAR threads that are not ET_NET. So the added assert should not ever trigger.

Still looks good to me.

@scw00 scw00 merged commit f5534ba into apache:master Feb 12, 2019
@scw00 scw00 deleted the none_server_session branch February 15, 2019 01:48
@bryancall bryancall modified the milestones: 9.0.0, 8.1.0 Mar 27, 2019
@bryancall
Copy link
Contributor

Cherry picked to 8.1.0

@zwoop zwoop modified the milestones: 8.1.0, 8.1.0-nogo Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants