You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/07/26 18:17:13 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
shinrich opened a new pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178
Ran across this while production testing H2 to origin. Details of that investigation in #7622.
The upshot is that the HTTP/2 logic does pay attention to the Connection header if the value is "closed". In the HTTP core logic, it regularly sets the "Connection: closed" header in the case of an error to ensure that in the HTTP/1.x case, the client or server connection is closed so any junk left on the socket will not be interpreted as a new transaction. I had been assuming that adding this header for HTTP/2 ua_txn's was a no-op since HTTP/2 doesn't use the Connection header.
But our implementation will take the presence of a Connection:closed header as an indication that it should start draining the HTTP/2 session. This PR replaces the logic that directly sets the Connection:closed header with a call to a virtual method. The HTTP/1.x version will set the header, and the default version does nothing.
https://docs.trafficserver.apache.org/en/latest/admin-guide/plugins/header_rewrite.en.html?highlight=connection%20close%20drain#close-connections-for-draining
With this change adding a header-rewrite rule to add the Connection:closed header will still work to trigger the draining logic. But core logic to clean up HTTP/1.x connections will not trigger the draining logic. In theory this change should increase the lifetime of HTTP/2 Sessions.
--
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] masaori335 commented on a change in pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178#discussion_r677888048
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -240,3 +240,11 @@ ProxyTransaction::allow_half_open() const
{
return false;
}
+
+// Most protocols will not want to set the Connection: header
+// For H2 it will initiate the drain logic. So we make do nothing
+// the default action.
Review comment:
For H2, I agree with we should not start the graceful shutdown. I'm a bit wondering about we should close the stream (send RST_STREAM frame to the peer).
--
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] masaori335 commented on a change in pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178#discussion_r678753392
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -7050,7 +7053,11 @@ HttpTransact::handle_response_keep_alive_headers(State *s, HTTPVersion ver, HTTP
case KA_CLOSE:
case KA_DISABLED:
if (s->client_info.keep_alive != HTTP_NO_KEEPALIVE || (ver == HTTP_1_1)) {
- heads->value_set(c_hdr_field_str, c_hdr_field_len, "close", 5);
+ if (s->client_info.proxy_connect_hdr) {
Review comment:
Thanks for the explanation, 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] masaori335 commented on a change in pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178#discussion_r677868004
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -7050,7 +7053,11 @@ HttpTransact::handle_response_keep_alive_headers(State *s, HTTPVersion ver, HTTP
case KA_CLOSE:
case KA_DISABLED:
if (s->client_info.keep_alive != HTTP_NO_KEEPALIVE || (ver == HTTP_1_1)) {
- heads->value_set(c_hdr_field_str, c_hdr_field_len, "close", 5);
+ if (s->client_info.proxy_connect_hdr) {
Review comment:
Why do we need to take care of the Proxy-Connection header here?
--
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 change in pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178#discussion_r678555513
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -240,3 +240,11 @@ ProxyTransaction::allow_half_open() const
{
return false;
}
+
+// Most protocols will not want to set the Connection: header
+// For H2 it will initiate the drain logic. So we make do nothing
+// the default action.
Review comment:
With the normal processing the stream will shutdown. With HTTP/1.1 the response is sent as normal even it if has Connection: close. With HTTP/2 after the response header and body are sent, the EOS bit will be sent on the last frame which will cause the stream to shutdown on the other side.
--
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 pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178#issuecomment-886996545
[approve ci clang-analyzer]
--
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 change in pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
Posted by GitBox <gi...@apache.org>.
shinrich commented on a change in pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178#discussion_r678556882
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -7050,7 +7053,11 @@ HttpTransact::handle_response_keep_alive_headers(State *s, HTTPVersion ver, HTTP
case KA_CLOSE:
case KA_DISABLED:
if (s->client_info.keep_alive != HTTP_NO_KEEPALIVE || (ver == HTTP_1_1)) {
- heads->value_set(c_hdr_field_str, c_hdr_field_len, "close", 5);
+ if (s->client_info.proxy_connect_hdr) {
Review comment:
This is to deal with the c_hdr_field_str. If you look a few lines up, it is either set to Proxy-connection or Connection depending on whether the parent selection path was taken or not. For H2 I assume we do want to set the proxy-connection (or at least that doesn't trigger the drain logic), so I added a test here to only call the new method in the Connection 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
[GitHub] [trafficserver] shinrich merged pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178
--
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] masaori335 commented on a change in pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
Posted by GitBox <gi...@apache.org>.
masaori335 commented on a change in pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178#discussion_r678747896
##########
File path: proxy/ProxyTransaction.cc
##########
@@ -240,3 +240,11 @@ ProxyTransaction::allow_half_open() const
{
return false;
}
+
+// Most protocols will not want to set the Connection: header
+// For H2 it will initiate the drain logic. So we make do nothing
+// the default action.
Review comment:
OK, then, do nothing :+1:
--
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 pull request #8178: Abstract adding Connection: close header to avoid triggering H2 draining logic
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8178:
URL: https://github.com/apache/trafficserver/pull/8178#issuecomment-887119369
[approve ci autest]
--
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