trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: git commit: TS-2555: updates to share lua context across hook invocations and only add global hooks if corresponding lua functions exist
Date Mon, 05 May 2014 16:06:45 GMT
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 <kichan@apache.org>
> Authored: Sun May 4 17:58:47 2014 -0700
> Committer: Kit Chan <kichan@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);
> }
> 


Mime
View raw message