You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by James Peach <jp...@apache.org> on 2014/05/05 18:06:45 UTC
Re: git commit: TS-2555: updates to share lua context across hook
invocations and only add global hooks if corresponding lua functions exist
On May 4, 2014, at 5:58 PM, kichan@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 <ki...@apache.org>
> Authored: Sun May 4 17:58:47 2014 -0700
> Committer: Kit Chan <ki...@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);
> }
>
Re: git commit: TS-2555: updates to share lua context across hook
invocations and only add global hooks if corresponding lua functions exist
Posted by Shu Kit Chan <ch...@gmail.com>.
>>> 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 <jp...@apache.org> wrote:
> On May 4, 2014, at 5:58 PM, kichan@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 <ki...@apache.org>
> > Authored: Sun May 4 17:58:47 2014 -0700
> > Committer: Kit Chan <ki...@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);
> > }
> >
>
>