You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2023/02/02 17:28:15 UTC
[GitHub] [trafficserver] bneradt opened a new pull request, #9366: Http2 to origin
bneradt opened a new pull request, #9366:
URL: https://github.com/apache/trafficserver/pull/9366
This implements the HTTP/2 to origin feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145143829
##########
tests/gold_tests/h2/http2.test.py:
##########
@@ -26,7 +26,7 @@
Test.SkipUnless(
Condition.HasCurlFeature('http2')
)
-Test.ContinueOnFail = True
+#Test.ContinueOnFail = True
Review Comment:
I'm not sure why these changes were made, but seems like something we want to have on 9.2 as well. If so, it's unfortunate that we can not simply cherry pick a commit to have these changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145201390
##########
proxy/http2/Http2Stream.h:
##########
@@ -334,9 +408,27 @@ Http2Stream::read_vio_writer() const
return this->read_vio.get_writer();
}
+inline bool
+Http2Stream::read_enabled() const
+{
+ return !this->read_vio.is_disabled();
+}
+
inline void
Http2Stream::_clear_timers()
{
_timeout.cancel_active_timeout();
_timeout.cancel_inactive_timeout();
}
+
+inline void
+Http2Stream::read_update(int count)
+{
+ read_vio.ndone += count;
+}
+
+inline void
+Http2Stream::read_done()
Review Comment:
Yes, this is good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1148015037
##########
proxy/http2/unit_tests/test_HTTP2.cc:
##########
@@ -107,8 +107,8 @@ TEST_CASE("Convert HTTPHdr", "[HTTP2]")
// check
CHECK_THAT(buf, Catch::StartsWith("GET https://trafficserver.apache.org/index.html HTTP/1.1\r\n"
- "Host: trafficserver.apache.org\r\n"
"User-Agent: foobar\r\n"
+ "Host: trafficserver.apache.org\r\n"
Review Comment:
Detaching field should not be that heavy. It's just changing a couple of flags, IIUC.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145087138
##########
proxy/http2/Http2CommonSession.cc:
##########
@@ -354,7 +355,8 @@ Http2CommonSession::do_process_frame_read(int event, VIO *vio, bool inside_frame
}
Http2ErrorCode err = Http2ErrorCode::HTTP2_ERROR_NO_ERROR;
- if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0)) {
+ if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0) &&
+ !this->is_outbound()) {
Review Comment:
I wonder why we don't enforce the error rate check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145119263
##########
proxy/http2/Http2Stream.h:
##########
@@ -295,6 +350,20 @@ Http2Stream::set_receive_headers(HTTPHdr &h2_headers)
_receive_header.copy(&h2_headers);
}
+inline void
+Http2Stream::reset_receive_headers()
+{
+ this->_receive_header.destroy();
+ this->_receive_header.create(HTTP_TYPE_RESPONSE);
Review Comment:
Don't we need to pass `HTTP_2_0`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145360885
##########
tests/gold_tests/h2/gold/nghttp_0_stdout.gold:
##########
@@ -12,5 +12,3 @@
``
[``] recv (stream_id=1) :status: 200
``
-``; END_STREAM
Review Comment:
This must have been an intermediate test change. It can be added back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145137317
##########
proxy/http2/unit_tests/test_HTTP2.cc:
##########
@@ -107,8 +107,8 @@ TEST_CASE("Convert HTTPHdr", "[HTTP2]")
// check
CHECK_THAT(buf, Catch::StartsWith("GET https://trafficserver.apache.org/index.html HTTP/1.1\r\n"
- "Host: trafficserver.apache.org\r\n"
"User-Agent: foobar\r\n"
+ "Host: trafficserver.apache.org\r\n"
Review Comment:
This is not great.
> A user agent that sends Host SHOULD send it as the first field in the header section of a request.
https://datatracker.ietf.org/doc/html/rfc9110#name-host-and-authority
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145274604
##########
tests/gold_tests/h2/http2.test.py:
##########
@@ -26,7 +26,7 @@
Test.SkipUnless(
Condition.HasCurlFeature('http2')
)
-Test.ContinueOnFail = True
+#Test.ContinueOnFail = True
Review Comment:
These changes can probably be reverted, actually. Definitely the ContinueOnFail comment was likely for Susan to debug something a while back. Let me revert them and if it causes problems in CI we can circle back around on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1106001176
##########
proxy/http2/Http2Stream.cc:
##########
@@ -520,35 +577,47 @@ Http2Stream::initiating_close()
bool sent_write_complete = false;
if (_sm) {
// Push out any last IO events
- if (write_vio.cont) {
+ // First look for active write or read
+ if (write_vio.cont && write_vio.nbytes > 0 && write_vio.ndone == write_vio.nbytes &&
Review Comment:
Thank you Kees. I appreciate your review and careful testing of this. I pulled in your patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] kspoelstra-aviatrix commented on pull request #9366: Http2 to origin
Posted by "kspoelstra-aviatrix (via GitHub)" <gi...@apache.org>.
kspoelstra-aviatrix commented on PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#issuecomment-1479215751
LGTM, I will look at trailers separately and continue testing it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145277498
##########
tests/gold_tests/h2/http2_flow_control.replay.yaml:
##########
@@ -88,9 +88,9 @@ sessions:
headers:
fields:
- [ X-Response, first-response ]
- - [ Content-Length, 28 ]
+ - [ Content-Length, 1200 ]
Review Comment:
Good question.
This test tests that ATS correctly handles session and stream windows correctly by passing large bodies with a variety of window sizes. Before HTTP/2 to origin, this test only was significant on the client side. Now we want to exercise these issues on the server side. Thus the body sizes of responses were increased. This is not helpful in 9.2 because we don't support HTTP/2 on the origin side there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145572612
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1790,19 +2090,20 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len
}
if (stream->is_write_vio_done()) {
+ Http2StreamDebug(this->session, stream->get_id(), "End of Data Frame");
flags |= HTTP2_FLAGS_DATA_END_STREAM;
}
// Update window size
- this->decrement_peer_rwnd_in(payload_length);
+ this->decrement_peer_rwnd(payload_length);
stream->decrement_peer_rwnd(payload_length);
// Create frame
- Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - client window con: %5zd stream: %5zd payload: %5zd",
- _peer_rwnd_in, stream->get_peer_rwnd(), payload_length);
+ Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - peer window con: %5zd stream: %5zd payload: %5zd flags: 0x%x",
+ _peer_rwnd, stream->get_peer_rwnd(), payload_length, flags);
Http2DataFrame data(stream->get_id(), flags, resp_reader, payload_length);
- this->session->xmit(data, flags & HTTP2_FLAGS_DATA_END_STREAM);
+ this->session->xmit(data);
Review Comment:
How delaying flush causes hanging? Sounds like the time-based flush logic is broken. Perhaps some event that used to be scheduled is no longer scheduled with this change and that takes away the opportunity to flush. I'm more worried about the behavior change than performance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145139872
##########
tests/gold_tests/h2/gold/nghttp_0_stdout.gold:
##########
@@ -12,5 +12,3 @@
``
[``] recv (stream_id=1) :status: 200
``
-``; END_STREAM
Review Comment:
I wonder why this was removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145584964
##########
proxy/http2/unit_tests/test_HTTP2.cc:
##########
@@ -107,8 +107,8 @@ TEST_CASE("Convert HTTPHdr", "[HTTP2]")
// check
CHECK_THAT(buf, Catch::StartsWith("GET https://trafficserver.apache.org/index.html HTTP/1.1\r\n"
- "Host: trafficserver.apache.org\r\n"
"User-Agent: foobar\r\n"
+ "Host: trafficserver.apache.org\r\n"
Review Comment:
Ah now I understand what you mean. We know there's ":authority" pseudo header before normal headers (if the request header is valid). How about renaming the field to Host?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1146957210
##########
proxy/http2/Http2CommonSession.cc:
##########
@@ -354,7 +355,8 @@ Http2CommonSession::do_process_frame_read(int event, VIO *vio, bool inside_frame
}
Http2ErrorCode err = Http2ErrorCode::HTTP2_ERROR_NO_ERROR;
- if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0)) {
+ if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0) &&
+ !this->is_outbound()) {
Review Comment:
I added the stream error rate check on outbound streams.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1149571320
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1790,19 +2090,20 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len
}
if (stream->is_write_vio_done()) {
+ Http2StreamDebug(this->session, stream->get_id(), "End of Data Frame");
flags |= HTTP2_FLAGS_DATA_END_STREAM;
}
// Update window size
- this->decrement_peer_rwnd_in(payload_length);
+ this->decrement_peer_rwnd(payload_length);
stream->decrement_peer_rwnd(payload_length);
// Create frame
- Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - client window con: %5zd stream: %5zd payload: %5zd",
- _peer_rwnd_in, stream->get_peer_rwnd(), payload_length);
+ Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - peer window con: %5zd stream: %5zd payload: %5zd flags: 0x%x",
+ _peer_rwnd, stream->get_peer_rwnd(), payload_length, flags);
Http2DataFrame data(stream->get_id(), flags, resp_reader, payload_length);
- this->session->xmit(data, flags & HTTP2_FLAGS_DATA_END_STREAM);
+ this->session->xmit(data);
Review Comment:
After some investigation, I was able to restore the conditional flush by removing the HTTP/2 WINDOW check logic that was added via this patch to `Http2Stream::update_write_request`. This passes all h2 flow control tests.
@shinrich : let me know whether you have any concerns about the `Remove window check from update_write_request` commit I just added for this PR. Currently that is this commit:
https://github.com/apache/trafficserver/pull/9366/commits/7c65be82299d491da076c452f6601eee3ab9e324
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145306687
##########
tests/gold_tests/h2/http2_flow_control.replay.yaml:
##########
@@ -88,9 +88,9 @@ sessions:
headers:
fields:
- [ X-Response, first-response ]
- - [ Content-Length, 28 ]
+ - [ Content-Length, 1200 ]
Review Comment:
I see. Makes sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145133641
##########
proxy/http2/Http2Stream.h:
##########
@@ -334,9 +408,27 @@ Http2Stream::read_vio_writer() const
return this->read_vio.get_writer();
}
+inline bool
+Http2Stream::read_enabled() const
+{
+ return !this->read_vio.is_disabled();
+}
+
inline void
Http2Stream::_clear_timers()
{
_timeout.cancel_active_timeout();
_timeout.cancel_inactive_timeout();
}
+
+inline void
+Http2Stream::read_update(int count)
+{
+ read_vio.ndone += count;
+}
+
+inline void
+Http2Stream::read_done()
Review Comment:
It's unclear what will be done by this function. `read_enabled` returns whether it's enabled but `read_done` doesn't return whether it's done.
I'd name these functions `is_read_enabled` `update_read_length` and `set _read_done`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145195085
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1790,19 +2090,20 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len
}
if (stream->is_write_vio_done()) {
+ Http2StreamDebug(this->session, stream->get_id(), "End of Data Frame");
flags |= HTTP2_FLAGS_DATA_END_STREAM;
}
// Update window size
- this->decrement_peer_rwnd_in(payload_length);
+ this->decrement_peer_rwnd(payload_length);
stream->decrement_peer_rwnd(payload_length);
// Create frame
- Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - client window con: %5zd stream: %5zd payload: %5zd",
- _peer_rwnd_in, stream->get_peer_rwnd(), payload_length);
+ Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - peer window con: %5zd stream: %5zd payload: %5zd flags: 0x%x",
+ _peer_rwnd, stream->get_peer_rwnd(), payload_length, flags);
Http2DataFrame data(stream->get_id(), flags, resp_reader, payload_length);
- this->session->xmit(data, flags & HTTP2_FLAGS_DATA_END_STREAM);
+ this->session->xmit(data);
Review Comment:
The second parameter is whether to flush, which defaults to true. In the previous patch we only flushed when the END_STREAM flag was set. I found situations via http2_flow_control.test.py where this would cause hanging if we didn't flush with each of these calls. So I changed it to always flush.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#issuecomment-1479805740
Let me quickly review the entire change once again. I'm not going to go into the detail this time but I'd like to make sure anything important isn't removed or commented out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145367229
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1790,19 +2090,20 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len
}
if (stream->is_write_vio_done()) {
+ Http2StreamDebug(this->session, stream->get_id(), "End of Data Frame");
flags |= HTTP2_FLAGS_DATA_END_STREAM;
}
// Update window size
- this->decrement_peer_rwnd_in(payload_length);
+ this->decrement_peer_rwnd(payload_length);
stream->decrement_peer_rwnd(payload_length);
// Create frame
- Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - client window con: %5zd stream: %5zd payload: %5zd",
- _peer_rwnd_in, stream->get_peer_rwnd(), payload_length);
+ Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - peer window con: %5zd stream: %5zd payload: %5zd flags: 0x%x",
+ _peer_rwnd, stream->get_peer_rwnd(), payload_length, flags);
Http2DataFrame data(stream->get_id(), flags, resp_reader, payload_length);
- this->session->xmit(data, flags & HTTP2_FLAGS_DATA_END_STREAM);
+ this->session->xmit(data);
Review Comment:
I didn't see issues until I added the extra http2.flow_control mechanisms that came about with this PR. Once things became more complicated between handling DATA frames and flow control states on both the client and server side, then I noticed that not always flushing was a problem. I don't think 9.2.x currently needs this, therefore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145123862
##########
proxy/http2/Http2Stream.h:
##########
@@ -295,6 +350,20 @@ Http2Stream::set_receive_headers(HTTPHdr &h2_headers)
_receive_header.copy(&h2_headers);
}
+inline void
+Http2Stream::reset_receive_headers()
+{
+ this->_receive_header.destroy();
+ this->_receive_header.create(HTTP_TYPE_RESPONSE);
Review Comment:
Maybe we don't need it because resetting is for trailer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145189836
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1424,6 +1697,17 @@ Http2ConnectionState::find_stream(Http2StreamId id) const
return nullptr;
}
+void
+Http2ConnectionState::start_streams()
Review Comment:
It is not. Removing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1142806599
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -2272,51 +2635,52 @@ Http2ConnectionState::_has_dynamic_stream_window() const
}
ssize_t
-Http2ConnectionState::get_peer_rwnd_in() const
+Http2ConnectionState::get_peer_rwnd() const
{
- return this->_peer_rwnd_in;
+ return this->_peer_rwnd;
}
Http2ErrorCode
-Http2ConnectionState::increment_peer_rwnd_in(size_t amount)
+Http2ConnectionState::increment_peer_rwnd(size_t amount)
{
- this->_peer_rwnd_in += amount;
+ this->_peer_rwnd += amount;
this->_recent_rwnd_increment[this->_recent_rwnd_increment_index] = amount;
++this->_recent_rwnd_increment_index;
this->_recent_rwnd_increment_index %= this->_recent_rwnd_increment.size();
- double sum = std::accumulate(this->_recent_rwnd_increment.begin(), this->_recent_rwnd_increment.end(), 0.0);
- double avg = sum / this->_recent_rwnd_increment.size();
- if (avg < Http2::min_avg_window_update) {
- HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_INSUFFICIENT_AVG_WINDOW_UPDATE, this_ethread());
- return Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM;
- }
+ // SKH Causing problems with gRPC processing. Python example resulted in amount 8
+ // double sum = std::accumulate(this->_recent_rwnd_increment.begin(), this->_recent_rwnd_increment.end(), 0.0);
+ // double avg = sum / this->_recent_rwnd_increment.size();
+ // if (avg < Http2::min_avg_window_update) {
+ // HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_INSUFFICIENT_AVG_WINDOW_UPDATE, this_ethread());
Review Comment:
This should not be removed because it's a security fix. If expected increments are small the threshold should be configured appropriately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1147951609
##########
proxy/http2/unit_tests/test_HTTP2.cc:
##########
@@ -107,8 +107,8 @@ TEST_CASE("Convert HTTPHdr", "[HTTP2]")
// check
CHECK_THAT(buf, Catch::StartsWith("GET https://trafficserver.apache.org/index.html HTTP/1.1\r\n"
- "Host: trafficserver.apache.org\r\n"
"User-Agent: foobar\r\n"
+ "Host: trafficserver.apache.org\r\n"
Review Comment:
This is a creative idea.
This actually turned out to be more challenging than I expected, though. Setting a field's name requires that the field be in the "detached" rather than the "live" state. At this point in the conversion, we are modifying the HTTPHdr and the fields are all live. I was able to avoid an append by first detaching the field, modifying the value, then re-attaching (which preserved the original order). I'm not sure what the performance implications of the detach operation are, however. It does look like the results are now functionally correct, however. The `:authority` header, with this latest patch, is now transformed into a `Host:` header and that `Host:` header is at the start of the header block after the request list.
Let me know whether you have any concerns with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145052878
##########
proxy/http/ConnectingEntry.h:
##########
@@ -0,0 +1,79 @@
+/** @file
+
+ Server side connection management.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+ */
+
+#include "PoolableSession.h"
+
+#include <set>
+#include <string>
+
+class HttpSM;
+
+/** Represents a server side session entry in a ConnectionPool to an origin. */
+class ConnectingEntry : public Continuation
+{
+public:
+ ConnectingEntry() = default;
+ ~ConnectingEntry() override;
+ void remove_entry();
+ int state_http_server_open(int event, void *data);
+ static PoolableSession *create_server_session(HttpSM *root_sm, NetVConnection *netvc, MIOBuffer *netvc_read_buffer,
+ IOBufferReader *netvc_reader);
+
+public:
+ std::string sni;
+ std::string cert_name;
+ IpEndpoint _ipaddr;
Review Comment:
Having `_` for public members is confusing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145112784
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1790,19 +2090,20 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len
}
if (stream->is_write_vio_done()) {
+ Http2StreamDebug(this->session, stream->get_id(), "End of Data Frame");
flags |= HTTP2_FLAGS_DATA_END_STREAM;
}
// Update window size
- this->decrement_peer_rwnd_in(payload_length);
+ this->decrement_peer_rwnd(payload_length);
stream->decrement_peer_rwnd(payload_length);
// Create frame
- Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - client window con: %5zd stream: %5zd payload: %5zd",
- _peer_rwnd_in, stream->get_peer_rwnd(), payload_length);
+ Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - peer window con: %5zd stream: %5zd payload: %5zd flags: 0x%x",
+ _peer_rwnd, stream->get_peer_rwnd(), payload_length, flags);
Http2DataFrame data(stream->get_id(), flags, resp_reader, payload_length);
- this->session->xmit(data, flags & HTTP2_FLAGS_DATA_END_STREAM);
+ this->session->xmit(data);
Review Comment:
I don't understand why we can remove the second parameter for END_STREAM flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145146472
##########
tests/gold_tests/h2/http2_flow_control.replay.yaml:
##########
@@ -88,9 +88,9 @@ sessions:
headers:
fields:
- [ X-Response, first-response ]
- - [ Content-Length, 28 ]
+ - [ Content-Length, 1200 ]
Review Comment:
I wonder why the size was increased. To make this test stable? Should we do this on 9.2 as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145339335
##########
proxy/http2/Http2Stream.h:
##########
@@ -295,6 +350,20 @@ Http2Stream::set_receive_headers(HTTPHdr &h2_headers)
_receive_header.copy(&h2_headers);
}
+inline void
+Http2Stream::reset_receive_headers()
+{
+ this->_receive_header.destroy();
+ this->_receive_header.create(HTTP_TYPE_RESPONSE);
Review Comment:
In short, the `_receive_header` is what is parsed from the wire, either request headers on the client side or response headers on the origin side, and the parsing logic (`http2_decode_header_blocks`) seems to only work if we do not "create" this with `HTTP_2_0`.
---
The `_receive_header` value is actually always always created uncategorized, not just here. See the also stream constructor here:
https://github.com/apache/trafficserver/pull/9366/files#diff-863a3cab1dd2ae149f1b54381d873d5291fb94f8aef77df7afb9b5f6a2d5dc61R66
See also the inbound version a few lines after that.
It is configured with the expected version through later processing of content received from the client or origin via decode_header_blocks. To explore your comment, I played around with an autest and gdb to see what happens if I initialize the `_receive_header` with `HTTP_2_0` and it seems like we mishandle pseudo header fields because they show up multiple times in the buffer. This causes us to fail a basic parsing check that says we should only have 4 or 1 pseudo headers, depending upon whether we are parsing a request or a response.
Note by contrast that our `_send_header` does not have this parsing problem because it is always explicitly crafted by us. As a result, `_send_header` is always created with `HTTP_2_0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145270792
##########
proxy/http2/unit_tests/test_HTTP2.cc:
##########
@@ -107,8 +107,8 @@ TEST_CASE("Convert HTTPHdr", "[HTTP2]")
// check
CHECK_THAT(buf, Catch::StartsWith("GET https://trafficserver.apache.org/index.html HTTP/1.1\r\n"
- "Host: trafficserver.apache.org\r\n"
"User-Agent: foobar\r\n"
+ "Host: trafficserver.apache.org\r\n"
Review Comment:
I see. This doesn't look trivial to fix.
When converting from h2 to h1, we take the `:authority` header and get a host value out of it and append a `Host:` header for the resultant HTTP/1 creation. Our `field_create` function appends and there is no corresponding function to prepend a header. Do you know a way offhand to prepend a header field at the beginning?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#issuecomment-1416616240
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] kspoelstra-aviatrix commented on a diff in pull request #9366: Http2 to origin
Posted by "kspoelstra-aviatrix (via GitHub)" <gi...@apache.org>.
kspoelstra-aviatrix commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1096107553
##########
proxy/http2/Http2Stream.cc:
##########
@@ -520,35 +577,47 @@ Http2Stream::initiating_close()
bool sent_write_complete = false;
if (_sm) {
// Push out any last IO events
- if (write_vio.cont) {
+ // First look for active write or read
+ if (write_vio.cont && write_vio.nbytes > 0 && write_vio.ndone == write_vio.nbytes &&
Review Comment:
This can possibly sent out a duplicate VC_EVENT_WRITE_COMPLETE/EOS to the HttpSM.
The HttpSM code is not setup to handle this, for example: if this happens after the tunnel_handler_post, we crash on the assert in tunnel_handler (expect event==EVENT_TUNNEL_DONE).
I have a fix running which has booleans for the read write vios, I'll add a pull request later
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1143534189
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -2272,51 +2635,52 @@ Http2ConnectionState::_has_dynamic_stream_window() const
}
ssize_t
-Http2ConnectionState::get_peer_rwnd_in() const
+Http2ConnectionState::get_peer_rwnd() const
{
- return this->_peer_rwnd_in;
+ return this->_peer_rwnd;
}
Http2ErrorCode
-Http2ConnectionState::increment_peer_rwnd_in(size_t amount)
+Http2ConnectionState::increment_peer_rwnd(size_t amount)
{
- this->_peer_rwnd_in += amount;
+ this->_peer_rwnd += amount;
this->_recent_rwnd_increment[this->_recent_rwnd_increment_index] = amount;
++this->_recent_rwnd_increment_index;
this->_recent_rwnd_increment_index %= this->_recent_rwnd_increment.size();
- double sum = std::accumulate(this->_recent_rwnd_increment.begin(), this->_recent_rwnd_increment.end(), 0.0);
- double avg = sum / this->_recent_rwnd_increment.size();
- if (avg < Http2::min_avg_window_update) {
- HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_INSUFFICIENT_AVG_WINDOW_UPDATE, this_ethread());
- return Http2ErrorCode::HTTP2_ERROR_ENHANCE_YOUR_CALM;
- }
+ // SKH Causing problems with gRPC processing. Python example resulted in amount 8
+ // double sum = std::accumulate(this->_recent_rwnd_increment.begin(), this->_recent_rwnd_increment.end(), 0.0);
+ // double avg = sum / this->_recent_rwnd_increment.size();
+ // if (avg < Http2::min_avg_window_update) {
+ // HTTP2_INCREMENT_THREAD_DYN_STAT(HTTP2_STAT_INSUFFICIENT_AVG_WINDOW_UPDATE, this_ethread());
Review Comment:
Makes sense. I added this back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145105982
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1424,6 +1697,17 @@ Http2ConnectionState::find_stream(Http2StreamId id) const
return nullptr;
}
+void
+Http2ConnectionState::start_streams()
Review Comment:
Where is this called?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145298435
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1790,19 +2090,20 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len
}
if (stream->is_write_vio_done()) {
+ Http2StreamDebug(this->session, stream->get_id(), "End of Data Frame");
flags |= HTTP2_FLAGS_DATA_END_STREAM;
}
// Update window size
- this->decrement_peer_rwnd_in(payload_length);
+ this->decrement_peer_rwnd(payload_length);
stream->decrement_peer_rwnd(payload_length);
// Create frame
- Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - client window con: %5zd stream: %5zd payload: %5zd",
- _peer_rwnd_in, stream->get_peer_rwnd(), payload_length);
+ Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - peer window con: %5zd stream: %5zd payload: %5zd flags: 0x%x",
+ _peer_rwnd, stream->get_peer_rwnd(), payload_length, flags);
Http2DataFrame data(stream->get_id(), flags, resp_reader, payload_length);
- this->session->xmit(data, flags & HTTP2_FLAGS_DATA_END_STREAM);
+ this->session->xmit(data);
Review Comment:
I think I added it to reduce the number of SSL_write calls with small payloads, and the buffer should be flushed if a certain amount of time elapsed since the last flush. And it's on 9.2.x. If there's a possibility of stall we should fix it separately so we can back port it to 9.2.x.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145302897
##########
proxy/http2/unit_tests/test_HTTP2.cc:
##########
@@ -107,8 +107,8 @@ TEST_CASE("Convert HTTPHdr", "[HTTP2]")
// check
CHECK_THAT(buf, Catch::StartsWith("GET https://trafficserver.apache.org/index.html HTTP/1.1\r\n"
- "Host: trafficserver.apache.org\r\n"
"User-Agent: foobar\r\n"
+ "Host: trafficserver.apache.org\r\n"
Review Comment:
`HTTPHdr::create()` receives HTTP version to deal with pseudo headers, which MUST be at the beginning. We may be able to do a similar thing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145380037
##########
proxy/http2/unit_tests/test_HTTP2.cc:
##########
@@ -107,8 +107,8 @@ TEST_CASE("Convert HTTPHdr", "[HTTP2]")
// check
CHECK_THAT(buf, Catch::StartsWith("GET https://trafficserver.apache.org/index.html HTTP/1.1\r\n"
- "Host: trafficserver.apache.org\r\n"
"User-Agent: foobar\r\n"
+ "Host: trafficserver.apache.org\r\n"
Review Comment:
It does add the pseudo header fields at the beginning, but it calls the same `field_attach` mechanism, which appends. It can do so because it is always on a new HdrHeap, it seems to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9366: Http2 to origin
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145383048
##########
proxy/http/ConnectingEntry.h:
##########
@@ -0,0 +1,79 @@
+/** @file
+
+ Server side connection management.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+
+ */
+
+#include "PoolableSession.h"
+
+#include <set>
+#include <string>
+
+class HttpSM;
+
+/** Represents a server side session entry in a ConnectionPool to an origin. */
+class ConnectingEntry : public Continuation
+{
+public:
+ ConnectingEntry() = default;
+ ~ConnectingEntry() override;
+ void remove_entry();
+ int state_http_server_open(int event, void *data);
+ static PoolableSession *create_server_session(HttpSM *root_sm, NetVConnection *netvc, MIOBuffer *netvc_read_buffer,
+ IOBufferReader *netvc_reader);
+
+public:
+ std::string sni;
+ std::string cert_name;
+ IpEndpoint _ipaddr;
Review Comment:
Agreed. I updated each of these accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a diff in pull request #9366: Http2 to origin
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145388843
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1790,19 +2090,20 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len
}
if (stream->is_write_vio_done()) {
+ Http2StreamDebug(this->session, stream->get_id(), "End of Data Frame");
flags |= HTTP2_FLAGS_DATA_END_STREAM;
}
// Update window size
- this->decrement_peer_rwnd_in(payload_length);
+ this->decrement_peer_rwnd(payload_length);
stream->decrement_peer_rwnd(payload_length);
// Create frame
- Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - client window con: %5zd stream: %5zd payload: %5zd",
- _peer_rwnd_in, stream->get_peer_rwnd(), payload_length);
+ Http2StreamDebug(session, stream->get_id(), "Send a DATA frame - peer window con: %5zd stream: %5zd payload: %5zd flags: 0x%x",
+ _peer_rwnd, stream->get_peer_rwnd(), payload_length, flags);
Http2DataFrame data(stream->get_id(), flags, resp_reader, payload_length);
- this->session->xmit(data, flags & HTTP2_FLAGS_DATA_END_STREAM);
+ this->session->xmit(data);
Review Comment:
So flushing always gives us correctness in the presence of the more sophisticated flow control options? Without the flush, the performance optimization of flushing less frequently causes stalls?
Sounds like the time-based flush logic doesn't work in with the more sophisticated flow control options. Perhaps we stick with the correct logic for this PR, and fix the performance optimization in another PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on a diff in pull request #9366: Http2 to origin
Posted by "shinrich (via GitHub)" <gi...@apache.org>.
shinrich commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145393026
##########
proxy/http2/Http2CommonSession.cc:
##########
@@ -354,7 +355,8 @@ Http2CommonSession::do_process_frame_read(int event, VIO *vio, bool inside_frame
}
Http2ErrorCode err = Http2ErrorCode::HTTP2_ERROR_NO_ERROR;
- if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0)) {
+ if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0) &&
+ !this->is_outbound()) {
Review Comment:
As I remember, I was seeing lots of spurious errors in the outbound case. Things that we were counting as errors, but weren't really. So we were closing things unnecessarily.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9366: Http2 to origin
Posted by "maskit (via GitHub)" <gi...@apache.org>.
maskit commented on code in PR #9366:
URL: https://github.com/apache/trafficserver/pull/9366#discussion_r1145555764
##########
proxy/http2/Http2CommonSession.cc:
##########
@@ -354,7 +355,8 @@ Http2CommonSession::do_process_frame_read(int event, VIO *vio, bool inside_frame
}
Http2ErrorCode err = Http2ErrorCode::HTTP2_ERROR_NO_ERROR;
- if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0)) {
+ if (this->connection_state.get_stream_error_rate() > std::min(1.0, Http2::stream_error_rate_threshold * 2.0) &&
+ !this->is_outbound()) {
Review Comment:
Why don't you fix the code that reports the spurious errors? I don't think we can ignore errors like this unless you can say real errors never happen in the outbound case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org