>>> How is it OK to reset the continuation data on a global continuation on each request? Other requests that are in flight are going to get their lua_State switched out from under them. What prevent multiple threads knocking on the same lua_State?
Yeah. I was thinking about this a bit and this probably will not work. Will be making some changes today. On Mon, May 5, 2014 at 9:06 AM, James Peach <jpe...@apache.org> wrote: > On May 4, 2014, at 5:58 PM, kic...@apache.org wrote: > > > Repository: trafficserver > > Updated Branches: > > refs/heads/master ea7849384 -> 5b1535e39 > > > > > > TS-2555: updates to share lua context across hook invocations and only > add global hooks if corresponding lua functions exist > > > > > > Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo > > Commit: > http://git-wip-us.apache.org/repos/asf/trafficserver/commit/5b1535e3 > > Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/5b1535e3 > > Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/5b1535e3 > > > > Branch: refs/heads/master > > Commit: 5b1535e399aa450eeb1c75e43c2398042036c363 > > Parents: ea78493 > > Author: Kit Chan <kic...@apache.org> > > Authored: Sun May 4 17:58:47 2014 -0700 > > Committer: Kit Chan <kic...@apache.org> > > Committed: Sun May 4 17:58:47 2014 -0700 > > > > ---------------------------------------------------------------------- > > plugins/experimental/ts_lua/ts_lua.c | 111 +++++++++++++++++++++++------- > > 1 file changed, 86 insertions(+), 25 deletions(-) > > ---------------------------------------------------------------------- > > > > > > > http://git-wip-us.apache.org/repos/asf/trafficserver/blob/5b1535e3/plugins/experimental/ts_lua/ts_lua.c > > ---------------------------------------------------------------------- > > diff --git a/plugins/experimental/ts_lua/ts_lua.c > b/plugins/experimental/ts_lua/ts_lua.c > > index 32cb58c..6c72a98 100644 > > --- a/plugins/experimental/ts_lua/ts_lua.c > > +++ b/plugins/experimental/ts_lua/ts_lua.c > > @@ -31,6 +31,8 @@ static volatile int32_t ts_lua_g_http_next_id = 0; > > ts_lua_main_ctx *ts_lua_main_ctx_array; > > ts_lua_main_ctx *ts_lua_g_main_ctx_array; > > These can be static. > > > > > +TSCont global_contp; > > This can be static. > > > + > > TSReturnCode > > TSRemapInit(TSRemapInterface *api_info, char * errbuf ATS_UNUSED , int > errbuf_size ATS_UNUSED ) > > { > > @@ -147,14 +149,11 @@ TSRemapDoRemap(void* ih, TSHttpTxn rh, > TSRemapRequestInfo *rri) > > return ret; > > } > > > > -static int > > -globalHookHandler(TSCont contp, TSEvent event, void *edata) { > > +static int > > +transactionStartHookHandler(TSCont contp, TSEvent event, void *edata) { > > TSHttpTxn txnp = (TSHttpTxn) edata; > > > > - int ret = 0; > > int64_t req_id; > > - > > - lua_State *l; > > TSCont txn_contp; > > > > ts_lua_main_ctx *main_ctx; > > @@ -165,12 +164,36 @@ globalHookHandler(TSCont contp, TSEvent event, > void *edata) { > > req_id = (int64_t) ts_lua_atomic_increment((&ts_lua_g_http_next_id), > 1); > > main_ctx = &ts_lua_g_main_ctx_array[req_id%TS_LUA_MAX_STATE_COUNT]; > > > > + TSDebug(TS_LUA_DEBUG_TAG, "[%s] req_id: %" PRId64, __FUNCTION__, > req_id); > > TSMutexLock(main_ctx->mutexp); > > > > http_ctx = ts_lua_create_http_ctx(main_ctx, conf); > > http_ctx->txnp = txnp; > > http_ctx->remap = 0; > > OK, so here you are hooking TS_HTTP_TXN_START_HOOK on every request as a > side-effect of simply loading the plugin. > > > + txn_contp = TSContCreate(ts_lua_http_cont_handler, NULL); > > + TSContDataSet(txn_contp, http_ctx); > > + http_ctx->main_contp = txn_contp; > > + TSHttpTxnHookAdd(txnp, TS_HTTP_TXN_CLOSE_HOOK, txn_contp); > > And then you set up a TS_HTTP_TXN_CLOSE_HOOK so that you can nuke the Lua > stat you just allocated. > > > + > > + TSContDataSet(global_contp, http_ctx); > > How is it OK to reset the continuation data on a global continuation on > each request? Other requests that are in flight are going to get their > lua_State switched out from under them. What prevent multiple threads > knocking on the same lua_State? > > > + TSMutexUnlock(main_ctx->mutexp); > > + > > + TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); > > + return 0; > > +} > > + > > +static int > > +globalHookHandler(TSCont contp, TSEvent event, void *edata) { > > The guts of this could be largely implemented by > ts_lua_http_cont_handler() AFAICT. > > > + TSHttpTxn txnp = (TSHttpTxn) edata; > > + > > + int ret = 0; > > + > > + lua_State *l; > > + > > + ts_lua_http_ctx *http_ctx = (ts_lua_http_ctx *) > TSContDataGet(contp); > > + > > TSMBuffer bufp; > > TSMLoc hdr_loc; > > TSMLoc url_loc; > > @@ -184,11 +207,10 @@ globalHookHandler(TSCont contp, TSEvent event, > void *edata) { > > } > > > > if(!http_ctx->client_request_hdrp) { > > - TSMutexUnlock(main_ctx->mutexp); > > TSHttpTxnReenable(txnp,TS_EVENT_HTTP_CONTINUE); > > return 0; > > } > > - > > + > > l = http_ctx->lua; > > > > switch (event) { > > @@ -213,22 +235,16 @@ globalHookHandler(TSCont contp, TSEvent event, > void *edata) { > > break; > > > > default: > > - TSMutexUnlock(main_ctx->mutexp); > > TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); > > return 0; > > break; > > } > > > > if (lua_type(l, -1) != LUA_TFUNCTION) { > > - TSMutexUnlock(main_ctx->mutexp); > > TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); > > Missing lua_pop. > > > return 0; > > } > > > > - txn_contp = TSContCreate(ts_lua_http_cont_handler, NULL); > > - TSContDataSet(txn_contp, http_ctx); > > - http_ctx->main_contp = txn_contp; > > - > > if (lua_pcall(l, 0, 1, 0) != 0) { > > fprintf(stderr, "lua_pcall failed: %s\n", lua_tostring(l, -1)); > > } > > @@ -236,10 +252,6 @@ globalHookHandler(TSCont contp, TSEvent event, void > *edata) { > > ret = lua_tointeger(l, -1); > > lua_pop(l, 1); > > > > - TSHttpTxnHookAdd(txnp, TS_HTTP_TXN_CLOSE_HOOK, txn_contp); > > - > > - TSMutexUnlock(main_ctx->mutexp); > > - > > TSHttpTxnReenable(txnp, TS_EVENT_HTTP_CONTINUE); > > return ret; > > } > > @@ -283,17 +295,66 @@ TSPluginInit(int argc, const char *argv[]) { > > return; > > } > > > > - TSCont global_contp = TSContCreate(globalHookHandler, NULL); > > + TSCont txn_start_contp = TSContCreate(transactionStartHookHandler, > NULL); > > + if (!txn_start_contp) { > > + TSError("[%s] could not create transaction start continuation", > __FUNCTION__ ); > > + return; > > + } > > + TSContDataSet(txn_start_contp, conf); > > + TSHttpHookAdd(TS_HTTP_TXN_START_HOOK, txn_start_contp); > > + > > + > > + global_contp = TSContCreate(globalHookHandler, NULL); > > if (!global_contp) { > > TSError("[%s] Could not create global continuation", __FUNCTION__); > > return; > > } > > - TSContDataSet(global_contp, conf); > > > > - TSHttpHookAdd(TS_HTTP_READ_REQUEST_HDR_HOOK, global_contp); > > - TSHttpHookAdd(TS_HTTP_SEND_REQUEST_HDR_HOOK, global_contp); > > - TSHttpHookAdd(TS_HTTP_READ_RESPONSE_HDR_HOOK, global_contp); > > - TSHttpHookAdd(TS_HTTP_SEND_RESPONSE_HDR_HOOK, global_contp); > > - TSHttpHookAdd(TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK, global_contp); > > - > > + //adding hook based on whether the lua global function exists. > > + ts_lua_main_ctx *main_ctx = &ts_lua_g_main_ctx_array[0]; > > + ts_lua_http_ctx *http_ctx = ts_lua_create_http_ctx(main_ctx, conf); > > + lua_State *l = http_ctx->lua; > > + int lua_function_exists = 0; > > + > > + lua_getglobal(l, TS_LUA_FUNCTION_G_SEND_REQUEST); > > + if(lua_type(l, -1) == LUA_TFUNCTION) { > > + lua_function_exists = 1; > > + TSHttpHookAdd(TS_HTTP_SEND_REQUEST_HDR_HOOK, global_contp); > > + TSDebug(TS_LUA_DEBUG_TAG, "send_request_hdr_hook added"); > > + } > > + lua_pop(l, 1); > > + > > + lua_getglobal(l, TS_LUA_FUNCTION_G_READ_RESPONSE); > > + if(lua_type(l, -1) == LUA_TFUNCTION) { > > + lua_function_exists = 1; > > + TSHttpHookAdd(TS_HTTP_READ_RESPONSE_HDR_HOOK, global_contp); > > + TSDebug(TS_LUA_DEBUG_TAG, "read_response_hdr_hook added"); > > + } > > + lua_pop(l, 1); > > + > > + lua_getglobal(l, TS_LUA_FUNCTION_G_SEND_RESPONSE); > > + if(lua_type(l, -1) == LUA_TFUNCTION) { > > + lua_function_exists = 1; > > + TSHttpHookAdd(TS_HTTP_SEND_RESPONSE_HDR_HOOK, global_contp); > > + TSDebug(TS_LUA_DEBUG_TAG, "send_response_hdr_hook added"); > > + } > > + lua_pop(l, 1); > > + > > + lua_getglobal(l, TS_LUA_FUNCTION_G_CACHE_LOOKUP_COMPLETE); > > + if(lua_type(l, -1) == LUA_TFUNCTION) { > > + lua_function_exists = 1; > > + TSHttpHookAdd(TS_HTTP_CACHE_LOOKUP_COMPLETE_HOOK, global_contp); > > + TSDebug(TS_LUA_DEBUG_TAG, "cache_lookup_complete_hook added"); > > + } > > + lua_pop(l, 1); > > + > > + lua_getglobal(l, TS_LUA_FUNCTION_G_READ_REQUEST); > > + if((lua_type(l, -1) == LUA_TFUNCTION) || lua_function_exists) { > > + lua_function_exists = 1; > > What is lua_function_exists doing here? I can't tell what makes > TS_HTTP_READ_REQUEST_HDR_HOOK special ... > > > + TSHttpHookAdd(TS_HTTP_READ_REQUEST_HDR_HOOK, global_contp); > > + TSDebug(TS_LUA_DEBUG_TAG, "read_request_hdr_hook added"); > > + } > > + lua_pop(l, 1); > > + > > + ts_lua_destroy_http_ctx(http_ctx); > > } > > > >
