You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by maskit <gi...@git.apache.org> on 2016/04/06 06:09:47 UTC
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
GitHub user maskit opened a pull request:
https://github.com/apache/trafficserver/pull/551
TS-4321: Keep response headers in FetchSM as they are
https://issues.apache.org/jira/browse/TS-4321
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/maskit/trafficserver ts4321
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/551.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 #551
----
commit e14a56a1179edc5d56acaf143a29f59df900c03e
Author: Masakazu Kitajo <mk...@yahoo-corp.jp>
Date: 2016-04-06T03:21:53Z
TS-4321: Keep response headers in FetchSM as they are
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-209208545
@maskit sound good 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by maskit <gi...@git.apache.org>.
Github user maskit closed the pull request at:
https://github.com/apache/trafficserver/pull/551
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-209208239
I'm going to push this fix without the test tomorrow, and I'll file a Jira for tests as a more general (but not too general) issue. Since tsqa has a test for chunked responses, we should reuse it.
I don't know test suites which can detect this. It seems that tsqa wraps h2spec, though it doesn't detect this.
Also, I think FetchSM should expose the headers with ``const`` modifier. We would need to make HTTPHdr usable with ``const`` first. I'll file a JIra for these too.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-206111521
Is this testable? It seems like we really need a regression test here ...
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/551#discussion_r58726866
--- Diff: proxy/http2/Http2ConnectionState.cc ---
@@ -1007,14 +1007,18 @@ Http2ConnectionState::send_headers_frame(FetchSM *fetch_sm)
DebugHttp2Stream(ua_session, stream->get_id(), "Send HEADERS frame");
- http2_convert_header_from_1_1_to_2(resp_header);
- buf_len = resp_header->length_get() * 2; // Make it double just in case
+ HTTPHdr *h2_hdr = http2_generate_h2_header_from_1_1(resp_header);
--- End diff --
I think we should probably avoid the malloc by using an out parameter for the HTTP2 copy.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-209756115
I filed three Jiras.
https://issues.apache.org/jira/browse/TS-4349
https://issues.apache.org/jira/browse/TS-4350
https://issues.apache.org/jira/browse/TS-4351
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-208206647
@jpeach I added a parameter to ``http2_generate_h2_header_from_1_1()`` to pass a ``HTTPHdr`` declared in ``send_headers_frame()`` instead of creating it with ``new`` in ``http2_generate_h2_header_from_1_1()``. Do I understand correctly?
I tried to write a test with tsqa but I haven't understood the test framework yet and some tests fail on my box. The test we need here is just send a request with HTTP/2, return a chunked response from an origin server, and then check the response body size. Could you help me out?
I think we should probably land this fix first without the regression test, because it's a 6.2.0 blocker.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/551#discussion_r59147631
--- Diff: proxy/http2/HTTP2.cc ---
@@ -498,25 +498,23 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers)
return PARSE_DONE;
}
-void
-http2_convert_header_from_1_1_to_2(HTTPHdr *headers)
+HTTPHdr *
+http2_generate_h2_header_from_1_1(HTTPHdr *headers)
--- End diff --
I wanted to do so. However, ``iter_get_first`` doesn't have ``const``.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-206128731
Though I tested it with nghttp and httpbin as an origin server, I think we can test with nghttp/curl and nc. But if you mean automated tests, I have no idea.
What do you think adding request/response data files as a first step so that we can test manually with nc and/or curl?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-209159096
I don't think we should hold this back to enforce a tsqa test. However, I'd suggest you file a separate Jira to add such a test. Curious: Does any of the other H2 test suites detect this? And is there a reasonable way to hook up those test suites to tsqa (i.e. a wrapper over those existing tests?).
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-209163324
Fwiw, I'm running with this patch now on {ci,docs}.trafficserver.apache.org, and it's fixing the issues I've seen on e.g. https://ci.trafficserver.apache.org/
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/551#discussion_r58726540
--- Diff: proxy/http2/HTTP2.cc ---
@@ -498,25 +498,23 @@ http2_convert_header_from_2_to_1_1(HTTPHdr *headers)
return PARSE_DONE;
}
-void
-http2_convert_header_from_1_1_to_2(HTTPHdr *headers)
+HTTPHdr *
+http2_generate_h2_header_from_1_1(HTTPHdr *headers)
--- End diff --
Make ``headers`` since you are not changing it here.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-208089304
Assigned to peach for review, and we really need this landed for 6.2.0, because it's completely crippling.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4321: Keep response headers in Fetc...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on the pull request:
https://github.com/apache/trafficserver/pull/551#issuecomment-206137778
I just remembered tsqa. I'll try to write a test for this with tsqa.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---