Repository: trafficserver Updated Branches: refs/heads/master d9aba01de -> 8abc04d13
TS-2988: ats_speed: bail out when gurl->IsWebValid() != true It seems that sometimes a url is received which can't be parsed as we valid by GoogleUrl(). Just ignore those requests, log them, and don't ASSERT on those. This commit also addresses another check failure I encountered in 5.x, seemingly caused by different behaviour compared to 4.x (which I used previously). Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/8abc04d1 Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/8abc04d1 Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/8abc04d1 Branch: refs/heads/master Commit: 8abc04d1383b18536f3950388829d22b4d884e8b Parents: d9aba01 Author: Otto van der Schaaf <[email protected]> Authored: Tue Aug 5 18:46:02 2014 +0200 Committer: Otto van der Schaaf <[email protected]> Committed: Tue Aug 5 18:46:02 2014 +0200 ---------------------------------------------------------------------- .../ats_speed/ats_beacon_intercept.cc | 10 +++- plugins/experimental/ats_speed/ats_speed.cc | 62 +++++++++++--------- 2 files changed, 41 insertions(+), 31 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/trafficserver/blob/8abc04d1/plugins/experimental/ats_speed/ats_beacon_intercept.cc ---------------------------------------------------------------------- diff --git a/plugins/experimental/ats_speed/ats_beacon_intercept.cc b/plugins/experimental/ats_speed/ats_beacon_intercept.cc index 175e218..1e97d8f 100644 --- a/plugins/experimental/ats_speed/ats_beacon_intercept.cc +++ b/plugins/experimental/ats_speed/ats_beacon_intercept.cc @@ -213,6 +213,12 @@ handleRead(InterceptCtx *cont_data, bool &read_complete) { static bool processRequest(InterceptCtx *cont_data) { + // OS: Looks like on 5.x we sometimes receive read complete / EOS events twice, + // which needs looking into. Probably this intercept is doing something it shouldn't + if (cont_data->output.buffer) { + TSDebug("ats_speed", "Received read complete / EOS twice?!"); + return true; + } string reply_header("HTTP/1.1 204 No Content\r\n"); int body_size = static_cast<int>(cont_data->body.size()); if (cont_data->req_content_len != body_size) { @@ -246,7 +252,7 @@ processRequest(InterceptCtx *cont_data) { net_instaweb::RequestContextPtr(system_request_context))) { TSError("Beacon handling failure!"); } else { - //TSDebug(DEBUG_TAG, "Beacon post data processed OK: [%s]", beacon_data.c_str()); + TSDebug(DEBUG_TAG, "Beacon post data processed OK: [%s]", beacon_data.c_str()); } cont_data->setupWrite(); @@ -329,7 +335,7 @@ txn_intercept(TSCont contp, TSEvent event, void *edata) { } delete cont_data; TSContDestroy(contp); - } + } return 1; } http://git-wip-us.apache.org/repos/asf/trafficserver/blob/8abc04d1/plugins/experimental/ats_speed/ats_speed.cc ---------------------------------------------------------------------- diff --git a/plugins/experimental/ats_speed/ats_speed.cc b/plugins/experimental/ats_speed/ats_speed.cc index afefa37..8cd96cb 100644 --- a/plugins/experimental/ats_speed/ats_speed.cc +++ b/plugins/experimental/ats_speed/ats_speed.cc @@ -716,40 +716,43 @@ handle_read_request_header(TSHttpTxn txnp) { ctx->url_string = new GoogleString(url, url_length); ctx->gurl = new GoogleUrl(*(ctx->url_string)); - CHECK(ctx->gurl->IsWebValid()) << "Invalid URL!"; - const char * method; - int method_len; - method = TSHttpHdrMethodGet(reqp, hdr_loc, &method_len); - bool head_or_get = method == TS_HTTP_METHOD_GET || method == TS_HTTP_METHOD_HEAD; - ctx->request_method = method; - GoogleString user_agent = get_header(reqp, hdr_loc, "User-Agent"); - ctx->user_agent = new GoogleString(user_agent); - ctx->server_context = ats_process_context->server_context(); - if (user_agent.find(kModPagespeedSubrequestUserAgent) != user_agent.npos) { - ctx->mps_user_agent = true; - } - if (ats_process_context->server_context()->IsPagespeedResource(gurl)) { - if (head_or_get && !ctx->mps_user_agent) { + if (!ctx->gurl->IsWebValid()) { + TSDebug("ats-speed", "URL != WebValid(): %s", ctx->url_string->c_str()); + } else { + const char * method; + int method_len; + method = TSHttpHdrMethodGet(reqp, hdr_loc, &method_len); + bool head_or_get = method == TS_HTTP_METHOD_GET || method == TS_HTTP_METHOD_HEAD; + ctx->request_method = method; + GoogleString user_agent = get_header(reqp, hdr_loc, "User-Agent"); + ctx->user_agent = new GoogleString(user_agent); + ctx->server_context = ats_process_context->server_context(); + if (user_agent.find(kModPagespeedSubrequestUserAgent) != user_agent.npos) { + ctx->mps_user_agent = true; + } + if (ats_process_context->server_context()->IsPagespeedResource(gurl)) { + if (head_or_get && !ctx->mps_user_agent) { + ctx->resource_request = true; + TSHttpTxnArgSet(txnp, TXN_INDEX_OWNED_ARG, &TXN_INDEX_OWNED_ARG_UNSET); + } + } else if (ctx->gurl->PathSansQuery() == "/pagespeed_message" + || ctx->gurl->PathSansQuery() == "/pagespeed_statistics" + || ctx->gurl->PathSansQuery() == "/pagespeed_global_statistics" + || ctx->gurl->PathSansQuery() == "/pagespeed_console" + || ctx->gurl->PathSansLeaf() == "/ats_speed_static/" + || ctx->gurl->PathSansQuery() == "/robots.txt" + ) { ctx->resource_request = true; TSHttpTxnArgSet(txnp, TXN_INDEX_OWNED_ARG, &TXN_INDEX_OWNED_ARG_UNSET); } - } else if (ctx->gurl->PathSansQuery() == "/pagespeed_message" - || ctx->gurl->PathSansQuery() == "/pagespeed_statistics" - || ctx->gurl->PathSansQuery() == "/pagespeed_global_statistics" - || ctx->gurl->PathSansQuery() == "/pagespeed_console" - || ctx->gurl->PathSansLeaf() == "/ats_speed_static/" - || ctx->gurl->PathSansQuery() == "/robots.txt" - ) { - ctx->resource_request = true; + else if (StringCaseEqual(gurl.PathSansQuery() ,"/ats_speed_beacon")) { + ctx->beacon_request = true; TSHttpTxnArgSet(txnp, TXN_INDEX_OWNED_ARG, &TXN_INDEX_OWNED_ARG_UNSET); - } - else if (StringCaseEqual(gurl.PathSansQuery() ,"/ats_speed_beacon")) { - ctx->beacon_request = true; - TSHttpTxnArgSet(txnp, TXN_INDEX_OWNED_ARG, &TXN_INDEX_OWNED_ARG_UNSET); - hook_beacon_intercept(txnp); + hook_beacon_intercept(txnp); + } } TSfree((void*)url); - } + } // gurl->IsWebValid() == true TSHandleMLocRelease(reqp, TS_NULL_MLOC, hdr_loc); } else { DCHECK(false) << "Could not get client request header\n"; @@ -868,7 +871,8 @@ transform_plugin(TSCont contp, TSEvent event, void *edata) TSHandleMLocRelease(response_header_buf, TS_NULL_MLOC, response_header_loc); } } - bool ok = !(ctx->resource_request || ctx->beacon_request || ctx->mps_user_agent); + bool ok = ctx->gurl->IsWebValid() && + !(ctx->resource_request || ctx->beacon_request || ctx->mps_user_agent); if (!ok) { TSHttpTxnReenable(txn, TS_EVENT_HTTP_CONTINUE); return 0;
