[GitHub] trafficserver issue #1627: TSHttpTxnAborted is not documented
GitHub user maskit opened an issue: https://github.com/apache/trafficserver/issues/1627 TSHttpTxnAborted is not documented TSHttpTxnAborted is not documented. It returns TSReturnCode (TS_SUCCESS | TS_ERROR), and it's very unclear whether a transaction is aborted or not. The truth is that it returns TS_SUCCESS if a transaction is aborted. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1612: Changed some of the HTTP/2 enums to enum classes ...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1612 @bryancall Looks good to me, but I don't see which part was the bug. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1607: Some headers are not sent on 408 timeout response...
GitHub user maskit opened an issue: https://github.com/apache/trafficserver/issues/1607 Some headers are not sent on 408 timeout responses Date, Via and Server header are not sent on 408 timeout response. Currently, only Connection header is sent. `HttpTransact::build_error_response()` stores the headers into `t_state.hdr_info.client_response.m_http` but they are not used. https://github.com/apache/trafficserver/blob/7.0.0/proxy/http/HttpSM.cc#L3497 Note that Connection header should always be present on 408. https://tools.ietf.org/html/rfc7231#section-6.5.7 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1604: TS-4976: Regularize plugins - protocol_stack
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1604 Please add Makefile.am. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1583: Set HttpSM::tunnel_handler_post to handle ...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1583#discussion_r107818082
--- Diff: proxy/http/HttpSM.cc ---
@@ -2771,7 +2771,8 @@ HttpSM::tunnel_handler_post(int event, void *data)
}
if (event != HTTP_TUNNEL_EVENT_DONE) {
-if ((event == VC_EVENT_WRITE_COMPLETE) || (event == VC_EVENT_EOS)) {
+if ((event == VC_EVENT_WRITE_COMPLETE) || (event == VC_EVENT_EOS) ||
(event == VC_EVENT_ERROR) ||
--- End diff --
I think we should add the inline function to make sure that the same
condition is used under the same context, and to clarify the condition we
expect.
The switch case style is more readable than current style though, it
doesn't tell us what the case is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1601: TS-4976: Regularize plugins - protocol
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1601#discussion_r107577489
--- Diff: example/protocol/Protocol.c ---
@@ -122,33 +122,29 @@ TSPluginInit(int argc, const char *argv[])
server_port = 4666;
if (argc < 3) {
-TSDebug("protocol", "Usage: protocol.so accept_port server_port");
+TSDebug(PLUGIN_NAME, "Usage: protocol.so accept_port server_port");
printf("[protocol_plugin] Usage: protocol.so accept_port
server_port\n");
--- End diff --
Should we keep these two?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1583: Set HttpSM::tunnel_handler_post to handle write e...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1583 @oknet The assert has gone. At least it works with H2. Thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1583: Set HttpSM::tunnel_handler_post to handle write e...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1583 I might be able to reproduce the issue, but only with H2. I couldn't reproduce it with H1. What I did are: - Add sleep(5) into consume() of PostBuffer plugin - Set proxy.config.http.transaction_no_activity_timeout_in and proxy.config.http.transaction_active_timeout_in very small (2~5 sec) - Apply the change on this PR - Run ATS with the modified PostBuffer plugin - Send a big POST request with curl The patch fixes an abort issue but ATS still aborts. (Fails at another assert.) ``` Fatal: HttpSM.cc:2725: failed assertion `post_transform_info.entry->in_tunnel == true` 2017-03-22 16:14:23.623562 traffic_server[68093:3635467] Fatal: HttpSM.cc:2725: failed assertion `post_transform_info.entry->in_tunnel == true` ``` ``` (lldb) bt * thread #14: tid = 0x37790b, 0x7fffd5191dd6 libsystem_kernel.dylib`__pthread_kill + 10, name = '[ET_NET 11]', stop reason = signal SIGABRT * frame #0: 0x7fffd5191dd6 libsystem_kernel.dylib`__pthread_kill + 10 frame #1: 0x7fffd527d787 libsystem_pthread.dylib`pthread_kill + 90 frame #2: 0x7fffd50f7420 libsystem_c.dylib`abort + 129 frame #3: 0x0002abd0 libtsutil.7.dylib`ink_abort(message_format="%s:%d: failed assertion `%s`") + 368 at ink_error.cc:99 frame #4: 0x00027a0f libtsutil.7.dylib`::_ink_assert(expression="post_transform_info.entry->in_tunnel == true", file="HttpSM.cc", line=2725) + 47 at ink_assert.cc:37 frame #5: 0x00010014ca3e traffic_server`HttpSM::tunnel_handler_post_or_put(this=0x05cfd9b0, p=0x05cfef00) + 174 at HttpSM.cc:2725 frame #6: 0x00010013fa06 traffic_server`HttpSM::tunnel_handler_post(this=0x05cfd9b0, event=103, data=0x03e2ded0) + 1062 at HttpSM.cc:2790 frame #7: 0x000100137708 traffic_server`HttpSM::main_handler(this=0x05cfd9b0, event=103, data=0x03e2ded0) + 776 at HttpSM.cc:2682 frame #8: 0x00010001ef60 traffic_server`Continuation::handleEvent(this=0x05cfd9b0, event=103, data=0x03e2ded0) + 112 at I_Continuation.h:153 frame #9: 0x0001001be69f traffic_server`Http2Stream::main_event_handler(this=0x03e2db40, event=103, edata=0x00b46aa0) + 1903 at Http2Stream.cc:88 frame #10: 0x00010001ef60 traffic_server`Continuation::handleEvent(this=0x03e2db40, event=103, data=0x00b46aa0) + 112 at I_Continuation.h:153 frame #11: 0x000100375a15 traffic_server`EThread::process_event(this=0x02d0c000, e=0x00b46aa0, calling_code=103) + 405 at UnixEThread.cc:143 frame #12: 0x0001003761dc traffic_server`EThread::execute(this=0x02d0c000) + 972 at UnixEThread.cc:240 frame #13: 0x000100374f57 traffic_server`spawn_thread_internal(a=0x037000c0) + 103 at Thread.cc:84 frame #14: 0x7fffd527aaab libsystem_pthread.dylib`_pthread_body + 180 frame #15: 0x7fffd527a9f7 libsystem_pthread.dylib`_pthread_start + 286 frame #16: 0x7fffd527a1fd libsystem_pthread.dylib`thread_start + 13 ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1591: Advertised header table size is not used
GitHub user maskit opened an issue: https://github.com/apache/trafficserver/issues/1591 Advertised header table size is not used Current implementation doesn't use the advertised header table size. To reproduce: ``` $ nghttp -nv -c 1024 https://localhost/ ``` This fails with COMPRESSION_ERROR. Todo: - Update internal maximum header table size with advertised size (SETTINGS_HEADER_TABLE_SIZE). - Send "Dynamic Table Size Update" in the beginning of a header block --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1502: issue #1501: reconstruct to load the default valu...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1502 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1590: Fix a wrong option charactar for access_ke...
Github user maskit closed the pull request at: https://github.com/apache/trafficserver/pull/1590 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1502: issue #1501: reconstruct to load the default valu...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1502 @zhilhuan The build issue is already fixed on master but the commit (c251f2ba72d0520d25f8099af78a540f1586f55b) is not in your branch, I think. Could you rebase again? Then all builds should succeed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1590: Fix a wrong option charactar for access_ke...
GitHub user maskit opened a pull request:
https://github.com/apache/trafficserver/pull/1590
Fix a wrong option charactar for access_key
The character doesn't match with line 536
```
{const_cast("access_key"), required_argument, nullptr, 'a'},
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/maskit/trafficserver fix_s3_auth
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1590.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1590
commit 6e9696770bea246d37d073af14686f7c0c06ce04
Author: Masakazu Kitajo <mas...@apache.org>
Date: 2017-03-17T03:36:01Z
Fix a wrong option charactar for access_key
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1502: issue #1501: reconstruct to load the default valu...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1502 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1585: Reset inactive timeout everytime H2 DATA f...
Github user maskit closed the pull request at: https://github.com/apache/trafficserver/pull/1585 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1585: Reset inactive timeout everytime H2 DATA frames a...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1585 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1585: Reset inactive timeout everytime H2 DATA f...
GitHub user maskit opened a pull request: https://github.com/apache/trafficserver/pull/1585 Reset inactive timeout everytime H2 DATA frames are sent The timer wasn't reset if transmission was resumed by WINDOW_UPDATE frames. You can merge this pull request into a Git repository by running: $ git pull https://github.com/maskit/trafficserver h2_inactive_timeout Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1585.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1585 commit 6559cc79239964e6d092d2c57e32a01ae4509a18 Author: Masakazu Kitajo <mas...@apache.org> Date: 2017-03-15T07:50:42Z Reset inactive timeout everytime H2 DATA frames are sent The timer wasn't reset if transmission was resumed by WINDOW_UPDATE frames. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1399: Paft of tcp_congestion_control code was erased by...
Github user maskit closed the issue at: https://github.com/apache/trafficserver/issues/1399 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1399: Paft of tcp_congestion_control code was erased by...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/issues/1399 Fixed on 6.2.x, 7.1.x and master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1426: issue #1399 add code of tcp_congestion_control er...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1426 @zwoop Yes, we should. I wrote the reason on #1399. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1534: Use StringView for protocol stack to avoid callin...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1534 I can see "h2" if I build it on Linux with gcc but not on Mac with clang. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1557: brotli support in gzip plugin
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1557 Having one plugin for multiple compression algorithms is fine with me. However, I'm not a big fun of adding it with "else if" statements, though we already did that for deflate. It looks like ifdef things and it's not so easy to read. I think it should have an abstract parent class for all the algorithms and each algorithms should be implemented as subclasses. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1534: Use StringView for protocol stack to avoid callin...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1534 It looks Http2ClientSession::PROTO_TAG is empty to me. I see two spaces between "http/1.1" and "tls/1.2". --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1534: Use StringView for protocol stack to avoid...
Github user maskit commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1534#discussion_r104723567 --- Diff: proxy/http/HttpTransactHeaders.cc --- @@ -685,6 +687,23 @@ HttpTransactHeaders::insert_server_header_in_response(const char *server_tag, in } } +/// Look up the protocol stack and write it to the @a via_string. +size_t +Write_Client_Protocol_Stack(HttpTransact::State *s, char *via_string, size_t len) --- End diff -- write_client_protocol_stack? Is this intentional? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1534: Use StringView for protocol stack to avoid...
Github user maskit commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1534#discussion_r104721541 --- Diff: proxy/http/HttpSM.cc --- @@ -8057,14 +8059,13 @@ HttpSM::is_redirect_required() // Fill in the client protocols used. Return the number of entries returned int -HttpSM::populate_client_protocol(const char **result, int n) const +HttpSM::populate_client_protocol(ts::StringView *result, int n) const --- End diff -- StringView (w/o namespace) ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1537: TS-4976: Regularize example plugin basic_auth.
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1537 Could you also update the documentation? ``` $ git grep basic-auth.c doc doc/developer-guide/plugins/example-plugins/basic-authorization/index.en.rst:The sample basic authorization plugin, ``basic-auth.c``, checks for doc/developer-guide/plugins/example-plugins/index.en.rst:and ``basic-auth.c``. doc/developer-guide/plugins/getting-started/index.en.rst:- ``basic-auth.c`` performs basic HTTP proxy authorization. doc/developer-guide/plugins/introduction.en.rst:``blacklist-1.c``, ``basic-auth.c``, and ``redirect-1.c``. ``` You can ignore `*.po` files. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1500: Create a keep alive session timeout and/or max # ...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/issues/1500 previous discussion: https://issues.apache.org/jira/browse/TS-4277 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1478: sscl is occasionally zero (formerly TS-2998)
Github user maskit commented on the issue: https://github.com/apache/trafficserver/issues/1478 It seems use of transform plugins (e.g. gzip plugin) cause this issue. I'm not sure why range requests matter here. https://github.com/apache/trafficserver/blob/7.1.x/proxy/http/HttpSM.cc#L3292-L3306 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1526: When SSL connect fails, we return 502 success
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1526 The fix seems reasonable, but why 7.1.1? It should be 7.2.0? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1520: Report protocol in request via header
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1520#discussion_r104074965
--- Diff: proxy/http/HttpTransactHeaders.cc ---
@@ -741,26 +741,14 @@
HttpTransactHeaders::insert_via_header_in_request(HttpTransact::State *s, HTTPHd
}
char *incoming_via = s->via_string;
- int scheme = s->orig_scheme;
- ink_assert(scheme >= 0);
-
- int scheme_len = hdrtoken_index_to_length(scheme);
- int32_t hversion = header->version_get().m_version;
-
- memcpy(via_string, hdrtoken_index_to_wks(scheme), scheme_len);
- via_string += scheme_len;
-
- // Common case (I hope?)
- if ((HTTP_MAJOR(hversion) == 1) && HTTP_MINOR(hversion) == 1) {
-memcpy(via_string, "/1.1 ", 5);
-via_string += 5;
- } else {
-*via_string++ = '/';
-*via_string++ = '0' + HTTP_MAJOR(hversion);
-*via_string++ = '.';
-*via_string++ = '0' + HTTP_MINOR(hversion);
+ char const *proto_buf[10]; // 10 seems like a reasonable number of
protos to print
+ int retval = s->state_machine->populate_client_protocol(proto_buf,
countof(proto_buf));
+ for (int i = 0; i < retval; i++) {
+memcpy(via_string, proto_buf[i], strlen(proto_buf[i]));
--- End diff --
I'm fine with use of StringView, but I think we should use it both request
side and response side. Why don't we do the change separately from this FIX?
Ideally, the logic should be unified.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1506: Wrong protocol version in the Via header
Github user maskit commented on the issue: https://github.com/apache/trafficserver/issues/1506 Just FYI: https://issues.apache.org/jira/browse/TS-4457 https://github.com/apache/trafficserver/pull/1066 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1485: back port "fix TS-4195: double free when stop tra...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1485 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1485: back port "fix TS-4195: double free when stop tra...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1485 @zizhong Could you include a reference for the original commit in your commit message? https://cwiki.apache.org/confluence/display/TS/Git#Git-Cherry-Pick --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1448: Avoid forcing "proxied URL" in case of transparen...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/issues/1448 @ykopel Could you send the patch as a pull request so we can run our CI build with your patch? I'm not familiar with transparent mode but the motivation and fix sound reasonable to me. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1399: Paft of tcp_congestion_control code was erased by...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/issues/1399 @shenzhang920 Thank you for the pull-requests and your patience. The fix has been merged on master and 6.2.x. @zwoop This is not a critical issue but I think we should backport the fix to 7.1 too, or it will be degradation when migrating from 6.2.2 to 7.1.x. Please let me know if you want me to send a pull-request for 7.1. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1425: PROXY_CONFIG_CONFIG_DIR doesn't change sysconfig ...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/issues/1425 `config_dir` is a read-only configuration. Some of configurations are not configurable at runtime. https://docs.trafficserver.apache.org/en/latest/admin-guide/files/records.config.en.html#proxy-config-config-dir Because overriding takes place WHILE (not before) reading `records.config`, changing `config_dir` with the environment variable is too late, though `traffic_layout` shows the new value. Even if you specify the new location in the config file, it wouldn't work due to the same reason, the file has already been open. I've never used `TS_ROOT`, but it might work. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1432: npn string should be updated on unregister...
Github user maskit closed the pull request at: https://github.com/apache/trafficserver/pull/1432 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1410: 6.2.x: TS-4665: H2 not terminating stream with sh...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1410 Done. This pull-request includes only TS-4665 now, since TS-4729 has been backported as a part of #1434. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1436: Backport f71b75e and 734aa31 from master to 6.2.x
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1436 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1431: CI builds fail on some platforms
GitHub user maskit opened an issue: https://github.com/apache/trafficserver/issues/1431 CI builds fail on some platforms Our CI builds fail on CentOS6 and Ubuntu12 because of an error occurred in check-unused-dependencies script. The reason of the error seems that the script couldn't find git command on the platforms. I guess it's a PATH issue. ``` FAIL: tools/check-unused-dependencies = Traceback (most recent call last): File "../tools/check-unused-dependencies", line 75, in for filename in subprocess.Popen(args, stdout=subprocess.PIPE).stdout: File "/usr/lib64/python2.6/subprocess.py", line 642, in __init__ errread, errwrite) File "/usr/lib64/python2.6/subprocess.py", line 1238, in _execute_child raise child_exception OSError: [Errno 2] No such file or directory ``` ``` 74 args = ['git', 'ls-files'] 75 for filename in subprocess.Popen(args, stdout=subprocess.PIPE).stdout: 76 filename = filename[:-1] ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1430: backport f71b75e and 734aa31 from master to 6.2.x
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1430 #1399 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1429: cherry-pick f71b75e from master to 6.2.x
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1429 OK, got it. Thanks. There are some ways to modify / replace commits. You could use `git commit --amend`, or reset the last commit and start the cherry-pick all over again. You'll have to use force push (`git push -f`) either way. :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1430: backport f71b75e and 734aa31 from master to 6.2.x
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1430 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1428: Fix a build issue that use of undeclared v...
Github user maskit closed the pull request at: https://github.com/apache/trafficserver/pull/1428 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1428: Fix a build issue that use of undeclared v...
GitHub user maskit opened a pull request: https://github.com/apache/trafficserver/pull/1428 Fix a build issue that use of undeclared variables The change made with #1426 causes build error on some platforms which doesn't support congestion control. I fixed the issue and changed code style a bit. You can merge this pull request into a Git repository by running: $ git pull https://github.com/maskit/trafficserver fix_1426 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1428.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1428 commit 35946ab4451644aa04d1a74dc3733b72e335628e Author: Masakazu Kitajo <mas...@apache.org> Date: 2017-02-09T01:13:50Z Fix a build issue that use of undeclared variables PR-1426 causes build error on some platforms which doesn't support congestion control. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1426: issue #1399 add code of tcp_congestion_con...
Github user maskit closed the pull request at: https://github.com/apache/trafficserver/pull/1426 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1415: isuue #1399 add back partial code of tcp_congesti...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1415 @shenzhang920 Thank you for the new pull-request. You can just close this pull request. After merging the new pull request, you can open a new one for 6.2.x with cherry-picked commit, or I can do it for you. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1426: issue #1399 add code of tcp_congestion_control er...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1426 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1415: isuue #1399 add back partial code of tcp_congesti...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1415 @shenzhang920 This should be fixed on master too, right? If there's the same issue on master, we should fix it on master first, and then we can cherry-pick the commit for 6.2.x. Could you make another pull-request for master? Sorry, I should have asked earlier. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1415: isuue #1399 add back partial code of tcp_congesti...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1415 [approve ci] --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1415: isuue #1399 add back partial code of tcp_congesti...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1415 @shenzhang920 I understand why you put the blocks there because the logic was originally in `new_connection()`. However, I'm not a big fun of putting the same code into two (or more) places. Is it possible to put the block into only one place? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1410: 6.2.x: TS-4729 and TS-4665: H2 not terminating st...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1410 Re-pushed with Conflicts info. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1410: 6.2.x: TS-4729 and TS-4665: H2 not termina...
GitHub user maskit opened a pull request: https://github.com/apache/trafficserver/pull/1410 6.2.x: TS-4729 and TS-4665: H2 not terminating stream with short chunked response Backporting #888. Just cherry-picked the two commits, and resolved few conflicts. You can merge this pull request into a Git repository by running: $ git pull https://github.com/maskit/trafficserver ts-4729-and-4665 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1410.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1410 commit 152205cd17c77fea92b246ddce3d6d2eb25a9875 Author: shinrich <shinr...@ieee.org> Date: 2016-08-10T20:00:43Z TS-4729: Fix dead assignment. (cherry picked from commit 29bc28a713463eb815524a91e03be95fe3aeedb1) commit f27dbeb22478f0a189ec2f79042e219479492f52 Author: Susan Hinrichs <shinr...@ieee.org> Date: 2016-08-22T14:33:57Z TS-4665: Http2 not terminating stream with short chunked response. (cherry picked from commit 5e9b18a1a8ed584e4e7cbbab9121b303b9465915) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1409: Log crc field and psql field correctly wit...
GitHub user maskit opened a pull request: https://github.com/apache/trafficserver/pull/1409 Log crc field and psql field correctly with H2 The two field values were logged incorrectly if a resopnse body is short. Because a write VIO isn't pass to a consumer if whole response body was sent before calling `update_write_request `, the field values aren't collected. The flag `retval` is used for switching whether a VIO will be passed. Also, 6.2.x has the same problem. You can merge this pull request into a Git repository by running: $ git pull https://github.com/maskit/trafficserver h2_logging Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/1409.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1409 commit b048dcd81d066f0fcb8b2f8b80fe25aa0ca164db Author: Masakazu Kitajo <mas...@apache.org> Date: 2017-02-01T23:54:52Z Log crc field and psql field correctly with H2 The two field values were logged incorrectly if a resopnse body is short. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1365: TS-4896: TSHttp***ClientAddrGet/TSHttp***Incoming...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1365 I think it should be backported to 7.1.x because returning NULL seems to be a sort of API breakage if old versions didn't return NULL in the same situation. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1365: TS-4896: TSHttp***ClientAddrGet/TSHttp***I...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1365#discussion_r97994347
--- Diff: proxy/ProxyClientSession.h ---
@@ -214,6 +214,19 @@ class ProxyClientSession : public VConnection
ink_hrtime ssn_start_time;
ink_hrtime ssn_last_txn_time;
+ virtual sockaddr const *
+ get_client_addr()
+ {
+NetVConnection *netvc = get_netvc();
+return netvc ? netvc->get_remote_addr() : NULL;
--- End diff --
nullptr?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1368: Issue #1367: HdrHeap potential corruption
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1368 I'm +1 on backporting to 7.1.x. I took a quick look and it seems to be easy, `git apply` fails though. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #1367: HdrHeap potential corruption
Github user maskit commented on the issue: https://github.com/apache/trafficserver/issues/1367 Yes, I solved the problem, but the coalescing issue is still alive. This kind of bugs can be made again until we get some measure for it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #888: TS-4665: Http2 not terminating stream with short c...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/888 @jaaju Unfortunately, this hasn't backported to 6.x yet, and it wouldn't be in 6.2.1 because the release is almost there (RC0 passed a voting). I'm going to try to backport in a couple of days, and hopefully, it will be in 6.2.2. Thank you for your patience. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1342: Fix CID 1368305: Dereference before null c...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1342#discussion_r97648065
--- Diff: plugins/experimental/money_trace/money_trace.cc ---
@@ -120,17 +120,13 @@ mt_check_request_header(TSHttpTxn txnp)
if (!hdr_value || length <= 0) {
LOG_DEBUG("ignoring, corrupt money trace header.");
} else {
-txn_data =
allocTransactionData();
-txn_data->client_request_mt_header = TSstrndup(hdr_value,
length);
-txn_data->client_request_mt_header[length] = '\0'; // workaround
for bug in core.
-LOG_DEBUG("found money trace header: %s, length: %d",
txn_data->client_request_mt_header, length);
if (nullptr == (contp = TSContCreate(transaction_handler,
nullptr))) {
LOG_ERROR("failed to create the transaction handler
continuation");
- if (nullptr != txn_data) {
-TSfree(txn_data->client_request_mt_header);
-TSfree(txn_data);
- }
} else {
+ txn_data =
allocTransactionData();
+ txn_data->client_request_mt_header =
TSstrndup(hdr_value, length);
--- End diff --
Well, then I think it should be checked in allocTransactionData() too, but
let's do that later if it is really needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1367: HdrHeap potential corruption
Github user maskit commented on the issue: https://github.com/apache/trafficserver/issues/1367 I faced a similar problem over 2 years ago. https://issues.apache.org/jira/browse/TS-2792 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #1067: TS-4914: Fix response headers on 304 respo...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1067#discussion_r97269280
--- Diff: proxy/http/HttpTransact.cc ---
@@ -7948,9 +7948,11 @@ HttpTransact::build_response(State *s, HTTPHdr
*base_response, HTTPHdr *outgoing
const char *value;
field = base_response->field_find(fields[i].name,
fields[i].len);
- ink_assert(field != nullptr);
- value = field->value_get();
- outgoing_response->value_append(fields[i].name,
fields[i].len, value, len, 0);
+ while (field) {
+value = field->value_get();
+outgoing_response->value_append(field_name[i],
field_len[i], value, len, true);
--- End diff --
This fix can be applicable for Cache-Control header and some other headers
but not for every headers.
Please refer to the Note in [RFC7230 Section
3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2). At least it's not
applicable for Set-Cookie header.
We might be able to have a blacklist / whitelist, but it won't be perfect
list. Maybe, just sending all headers separately is a safer way?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1342: Fix CID 1368305: Dereference before null c...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1342#discussion_r97264664
--- Diff: plugins/experimental/money_trace/money_trace.cc ---
@@ -120,17 +120,13 @@ mt_check_request_header(TSHttpTxn txnp)
if (!hdr_value || length <= 0) {
LOG_DEBUG("ignoring, corrupt money trace header.");
} else {
-txn_data =
allocTransactionData();
-txn_data->client_request_mt_header = TSstrndup(hdr_value,
length);
-txn_data->client_request_mt_header[length] = '\0'; // workaround
for bug in core.
-LOG_DEBUG("found money trace header: %s, length: %d",
txn_data->client_request_mt_header, length);
if (nullptr == (contp = TSContCreate(transaction_handler,
nullptr))) {
LOG_ERROR("failed to create the transaction handler
continuation");
- if (nullptr != txn_data) {
-TSfree(txn_data->client_request_mt_header);
-TSfree(txn_data);
- }
} else {
+ txn_data =
allocTransactionData();
+ txn_data->client_request_mt_header =
TSstrndup(hdr_value, length);
--- End diff --
Shouldn't we check if `txn_data` is not null?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1262: TS-5092: ATS handling of too many concurre...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1262#discussion_r94286274
--- Diff: proxy/http2/Http2ConnectionState.cc ---
@@ -728,18 +732,18 @@ rcv_continuation_frame(Http2ConnectionState ,
const Http2Frame )
cstate.clear_continued_stream_id();
if (!stream->change_state(HTTP2_FRAME_TYPE_CONTINUATION,
frame.header().flags)) {
- return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_PROTOCOL_ERROR);
+ return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_PROTOCOL_ERROR, "continuation no state change");
}
Http2ErrorCode result =
stream->decode_header_blocks(*cstate.local_hpack_handle);
if (result != HTTP2_ERROR_NO_ERROR) {
if (result == HTTP2_ERROR_COMPRESSION_ERROR) {
-return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_COMPRESSION_ERROR);
+return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_COMPRESSION_ERROR, "continuation compression error");
} else if (result == HTTP2_ERROR_ENHANCE_YOUR_CALM) {
-return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_ENHANCE_YOUR_CALM);
+return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_ENHANCE_YOUR_CALM, "continuation enhance your calm");
} else {
-return Http2Error(HTTP2_ERROR_CLASS_STREAM,
HTTP2_ERROR_PROTOCOL_ERROR);
+return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_PROTOCOL_ERROR, "continuation decode error");
--- End diff --
This case is actually not for a decode error but HTTP2 violation, such as
using uppercase in header names. All decode errors should be handled as
compression error. We don't have to abandon all following requests with a
connection error, because the HPACK decoder instance keeps valid status. Please
see 8.1.2.6.
I'd put a message like "continuation malformed request".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1262: TS-5092: ATS handling of too many concurre...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1262#discussion_r94285648
--- Diff: proxy/http2/Http2ConnectionState.cc ---
@@ -127,10 +128,11 @@ rcv_data_frame(Http2ConnectionState , const
Http2Frame )
// Check whether Window Size is acceptable
if (cstate.server_rwnd < payload_length) {
-return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_FLOW_CONTROL_ERROR);
+return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_FLOW_CONTROL_ERROR,
+ "recv data cstate.server_rwnd < payload_length");
}
if (stream->server_rwnd < payload_length) {
-return Http2Error(HTTP2_ERROR_CLASS_STREAM,
HTTP2_ERROR_FLOW_CONTROL_ERROR);
+return Http2Error(HTTP2_ERROR_CLASS_STREAM,
HTTP2_ERROR_FLOW_CONTROL_ERROR, "recvdata stream->server_rwnd <
payload_length");
--- End diff --
"recv data" -- please add a space.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1262: TS-5092: ATS handling of too many concurre...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1262#discussion_r94285819
--- Diff: proxy/http2/Http2ConnectionState.cc ---
@@ -683,8 +686,10 @@ rcv_continuation_frame(Http2ConnectionState ,
const Http2Frame )
DebugHttp2Stream(cstate.ua_session, stream_id, "Received CONTINUATION
frame");
+ // Error("http2_cs [%" PRId64 "] Received RST_STREAM frame for %d",
cs.connection_id(), stream_id);
--- End diff --
This line should not be necessary.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1262: TS-5092: ATS handling of too many concurre...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1262#discussion_r94286295
--- Diff: proxy/http2/Http2ConnectionState.cc ---
@@ -301,11 +304,11 @@ rcv_headers_frame(Http2ConnectionState , const
Http2Frame )
if (result != HTTP2_ERROR_NO_ERROR) {
if (result == HTTP2_ERROR_COMPRESSION_ERROR) {
-return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_COMPRESSION_ERROR);
+return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_COMPRESSION_ERROR, "recv headers compression error");
} else if (result == HTTP2_ERROR_ENHANCE_YOUR_CALM) {
-return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_ENHANCE_YOUR_CALM);
+return Http2Error(HTTP2_ERROR_CLASS_CONNECTION,
HTTP2_ERROR_ENHANCE_YOUR_CALM, "recv headers enhance your calm");
} else {
-return Http2Error(HTTP2_ERROR_CLASS_STREAM,
HTTP2_ERROR_PROTOCOL_ERROR);
+return Http2Error(HTTP2_ERROR_CLASS_STREAM,
HTTP2_ERROR_PROTOCOL_ERROR, "recv headers decode error");
--- End diff --
This is not a decode error. Please see a comment for the same logic in
rcv_continuation_frame.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1160: TS-5019: Add header length checks in HPACK
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/1160 Looks good. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #833: TS-3474: HTTP/2 Server Push support
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/833 Because I thought it's the easiest way. I could parse query parameters and use a specific parameter as a trigger but it needs more codes. I wanted to keep the example plugin simple. I'd happy to rewrite the function if you have better ideas. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #833: TS-3474: HTTP/2 Server Push support
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/833 @bryancall That's odd. I just rebased again but I don't see any problems. > After removing TSUrlHttpQueryGet() from the plugin I am seeing many server pushes for the URL in the logs. If you simply removed the line, this is understandable. I guess the variable is not initialized and have some value other than 0. It triggers many server pushes, because the sample plugin is using query string length stored in the variable. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #972: TS-4459: Force domain names in cert to be lowercas...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/972 It seems reasonable. ð --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #999: TS-4759: Fix stream states management
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/999 Looks OK. I understand the necessity of doing this but I'm not thrilled adding more state flags to Http2Stream class. I can live with them but I hope they are managed in `_state` in the future. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #833: TS-3474: HTTP/2 Server Push support
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/833 It works now. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #833: TS-3474: HTTP/2 Server Push support
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/833 It doesn't work correctly after the rebasing. I'm working on it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #913: TS-2237: Add unit tests for escapify_url an...
Github user maskit closed the pull request at: https://github.com/apache/trafficserver/pull/913 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #913: TS-2237: Add unit tests for escapify_url and pure_...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/913 Rebased to obtain the functions from master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #833: TS-3474: HTTP/2 Server Push support
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/833 Rebased for now. We still needs discuss about `url` argument. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #951: TS-4797: Allow backslash-escape in header_rewrite ...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/951 :+1: --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #951: TS-4797: Allow backslash-escape in header_rewrite ...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/951 As I commented inline, this introduces an unexpected behavior. I suggest using the conservative change I wrote on the comment. Also, it's a trivial thing but I'd suggest using "bar" instead of "var". --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #951: TS-4797: Allow backslash-escape in header_r...
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/951#discussion_r77433626
--- Diff: plugins/header_rewrite/parser.cc ---
@@ -50,6 +52,12 @@ Parser::Parser(const std::string ) : _cond(false),
_empty(false)
_tokens.push_back(std::string(1, line[i]));
}
continue; /* always eat whitespace */
+} else if (line[i] == '\\') {
+ // erase a backslash in quoted-string
+ if (inquote && extracting_token) {
--- End diff --
Without this change, the test below passes.
```
{
ParserTest p("add-header foo \\var\\");
CHECK_EQ(p.getTokens().size(), 3);
CHECK_EQ(p.getTokens()[0], "add-header");
CHECK_EQ(p.getTokens()[1], "foo");
CHECK_EQ(p.getTokens()[2], "\\var\\");
}
```
But with this change, it fails.
The test case will need to be changed like:
```
CHECK_EQ(p.getTokens()[2], "var\\");
```
This seems very odd. Even if there is no actual use case, it's hard to
understand the behavior.
However, it seems we don't support backslash-escaping completely outside of
a quoted string string now, actually.
```
// This doesn't work but it's OK, maybe.
{
ParserTest p("add-header foo ");
CHECK_EQ(p.getTokens().size(), 3);
}
// This doesn't work even without Masaori's change. I guess this is the
case James pointed out.
{
ParserTest p("add-header foo \\<bar\\>");
CHECK_EQ(p.getTokens().size(), 3);
}
// This works either way.
{
ParserTest p("add-header foo \"\"");
CHECK_EQ(p.getTokens().size(), 3);
CHECK_EQ(p.getTokens()[0], "add-header");
CHECK_EQ(p.getTokens()[1], "foo");
CHECK_EQ(p.getTokens()[2], "");
}
```
I don't know whether this is a regression.
Anyway, more conservative codes would be:
```
if (!extracting_token) {
extracting_token = true;
cur_token_start = i;
}
if (inquote) {
line.erase(i, 1);
continue;
}
```
I think we should add all test cases above to clarify the cases now we
support.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #921: TS-4776: Emulate the effect of O_DIRECTORY ...
Github user maskit closed the pull request at: https://github.com/apache/trafficserver/pull/921 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #921: TS-4776: Emulate the effect of O_DIRECTORY if it i...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/921 Waiting +1s. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #921: TS-4776: Emulate the effect of O_DIRECTORY if it i...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/921 @jacksontj Could you take a look at this please? Because the line seems to be added with your PR #653. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #921: TS-4776: Emulate the effect of O_DIRECTORY ...
GitHub user maskit opened a pull request: https://github.com/apache/trafficserver/pull/921 TS-4776: Emulate the effect of O_DIRECTORY if it is not defined Emulate the effect of `O_DIRECTORY` with `stat()` if it is not defined. According to man page of `open()`, it returns `ENOTDIR` if the specified pass was not a directory. > ENOTDIR > A component used as a directory in pathname is not, in fact, a directory, or O_DIRECTORY was specified and pathname was not a directory. You can merge this pull request into a Git repository by running: $ git pull https://github.com/maskit/trafficserver ts4776 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/921.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #921 commit e3c127f608675ab07cdda414cac8a5c5367e5608 Author: Masakazu Kitajo <mas...@apache.org> Date: 2016-08-25T09:34:59Z TS-4776: Emulate the effect of O_DIRECTORY if it is not defined --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #913: TS-2237: Add unit tests for escapify_url and pure_...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/913 CI builds fail because the functions are not in the code yet. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #913: TS-2237: Add unit tests for escapify_url an...
GitHub user maskit opened a pull request: https://github.com/apache/trafficserver/pull/913 TS-2237: Add unit tests for escapify_url and pure_escapify_url Unit tests for #866 You can merge this pull request into a Git repository by running: $ git pull https://github.com/maskit/trafficserver ts2237 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/913.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #913 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #886: TS-4217: Change stream state after sending ...
Github user maskit closed the pull request at: https://github.com/apache/trafficserver/pull/886 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #907: TS-3826: Traffic Server adds body "No Content" to ...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/907 ð --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #895: TS-4700: Change the default timeout for HTTP/2
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/895 I'm on the same page with @zwoop . But the change seems reasonable. ð --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #888: TS-4665: Http2 not terminating stream with short c...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/888 Reviewed quickly. Looks ok, but I'm not sure about this change. +0 from me. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #885: TS-4546: Fix a build error on OmniOS
Github user maskit closed the pull request at: https://github.com/apache/trafficserver/pull/885 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 ð --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #885: TS-4546: Fix a build error on OmniOS
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/885 Updated. - Use PATH_MAX - Use ink_strlcat --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #886: TS-4217: Change stream state after sending ...
GitHub user maskit opened a pull request: https://github.com/apache/trafficserver/pull/886 TS-4217: Change stream state after sending HEADERS frame Change stream state to CLOSED after sending a HEADERS w/ END_STREAM flag to avoid sending a unnecessary DATA frame w/ END_STREAM flag. You can merge this pull request into a Git repository by running: $ git pull https://github.com/maskit/trafficserver ts4217 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/886.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #886 commit f5c2a2d8d3b4dd4afc1c32ec32627d56c16f78e0 Author: Masakazu Kitajo <mas...@apache.org> Date: 2016-08-21T14:15:28Z TS-4217: Change stream state after sending HEADERS frame Change stream state to CLOSED after sending a HEADERS w/ END_STREAM flag to avoid sending a unnecessary DATA frame w/ END_STREAM flag. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 > are you ok with the change if we make a new version of LogUtils::escapify_url that does not include this change that is used by TSStringPercentEncode? So the change only affects core logic? @shinrich Yes, I'm fine with that. I would file another JIRA to track down the root cause of double-encoding. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver pull request #885: TS-4546: Fix a build error on OmniOS
GitHub user maskit opened a pull request: https://github.com/apache/trafficserver/pull/885 TS-4546: Fix a build error on OmniOS https://issues.apache.org/jira/browse/TS-4546 OmniOS doesn't have d_type in dirent structure, so use stat() instead. You can merge this pull request into a Git repository by running: $ git pull https://github.com/maskit/trafficserver ts4546 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/trafficserver/pull/885.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #885 commit 651d98a8a7e3c886530fdcfeeb0e0e8399569a80 Author: Masakazu Kitajo <mas...@apache.org> Date: 2016-08-21T05:21:08Z TS-4546: Fix a build error on OmniOS OmniOS doesn't have d_type in dirent structure, so use stat() instead. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 I'd say it is not sufficient. As a workaround for most cases, it works, and personally I'm OK with it as is. However, I think the behavior of `TSStringPercentEncode` shouldn't be changed because it's just a workaround for logging issue. So, if the API keeps current behavior, then I'm fine with landing this change. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 ``HttpRequestData::get_string()`` unescapes an URL and it is called from ``UrlMatcher<Data, Result>::Match(RequestData *rdata, Result *result)``. I think this is the code Sudheer mentioned. However, at least, the unescaped URL doesn't come out from the function. If we could assure that no unescaped string flows into the logging system, we will be able to simply remove some of calls of ``LogUtils::url_escapify``. Also, I realized that ``TSStringPercentEncode`` uses ``LogUtils::url_escapify`` internally. So changing behavior of ``LogUtils::url_escapify`` would affect plugins. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 I wrote a unit test for CURRENT `escapify_url`. http://pastebin.com/XZ4x8bKg With your change, you would need to change the last expected value in the test cases. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...
Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 I don't think this is the right approach. With this change, "%%20" will be encoded to "%25%20", right? What if "%%20" was not encoded string? It should be encoded to "%25%2520". Shouldn't we make sure that all callers of this function pass decoded URLs? Another options is to abort encoding and return inputs as outputs if input URLs seem to be already encoded. It can't handle mixed cases but I think it would't happen. (If it happens, it should be a bug.) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
