maskit commented on a change in pull request #7622:
URL: https://github.com/apache/trafficserver/pull/7622#discussion_r603038536
##########
File path: iocore/net/UnixNetVConnection.cc
##########
@@ -431,6 +432,9 @@ write_to_net_io(NetHandler *nh, UnixNetVConnection *vc,
EThread *thread)
if (towrite != ntodo && buf.writer()->write_avail()) {
if (write_signal_and_update(VC_EVENT_WRITE_READY, vc) != EVENT_CONT) {
return;
+ } else if (c != s->vio.cont) { /* The write vio was updated in the handler
*/
Review comment:
Nitpick: Do we want variable `c`?
##########
File path: proxy/http/HttpTransact.h
##########
@@ -907,6 +901,16 @@ class HttpTransact
}
ProxyProtocol pp_info;
Review comment:
I want an empty line here. I misread this as a return type.
##########
File path: proxy/logging/LogAccess.cc
##########
@@ -1782,6 +1782,40 @@ LogAccess::marshal_client_req_protocol_version(char *buf)
return len;
}
+/*-------------------------------------------------------------------------
+ -------------------------------------------------------------------------*/
+
+int
+LogAccess::marshal_server_req_protocol_version(char *buf)
Review comment:
Not a big deal, but I think this part is reviewable even if a reviewer
is not so familiar with H2 and server session. I'd be happy to add the new log
field beforehand as a preparatory step.
##########
File path: tests/gold_tests/continuations/openclose.test.py
##########
@@ -51,7 +52,7 @@
ts.Variables.port, server.Variables.Port)
)
-cmd = 'curl -vs -H "host:oc.test"
http://127.0.0.1:{0}'.format(ts.Variables.port)
+cmd = 'curl -H "Connection: close" -vs -H "host:oc.test"
http://127.0.0.1:{0}'.format(ts.Variables.port)
Review comment:
I'd like to make this change before landing H2 to Origin. That would
ensure H2 from Client and H1 to Origin are not broken.
##########
File path: proxy/http/HttpTransact.h
##########
@@ -582,7 +576,7 @@ class HttpTransact
ConnectionAttributes *server = nullptr;
ink_time_t now = 0;
ServerState_t state = STATE_UNDEFINED;
- unsigned attempts = 1;
+ unsigned attempts = 0;
Review comment:
Is this intentional change?
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -3969,47 +3977,21 @@
HttpTransact::handle_forward_server_connection_open(State *s)
TxnDebug("http_seq", "[HttpTransact::handle_server_connection_open] ");
ink_release_assert(s->current.state == CONNECTION_ALIVE);
- if (s->hdr_info.server_response.version_get() == HTTPVersion(0, 9)) {
- TxnDebug("http_trans", "[hfsco] server sent 0.9 response, reading...");
- build_response(s, &s->hdr_info.client_response,
s->client_info.http_version, HTTP_STATUS_OK, "Connection Established");
-
- s->client_info.keep_alive = HTTP_NO_KEEPALIVE;
- s->cache_info.action = CACHE_DO_NO_ACTION;
- s->next_action = SM_ACTION_SERVER_READ;
- return;
-
- } else if (s->hdr_info.server_response.version_get() == HTTPVersion(1, 0)) {
- if (s->current.server->http_version == HTTPVersion(0, 9)) {
- // update_hostdb_to_indicate_server_version_is_1_0
- s->updated_server_version = HostDBApplicationInfo::HTTP_VERSION_10;
- } else if (s->current.server->http_version == HTTPVersion(1, 1)) {
- // update_hostdb_to_indicate_server_version_is_1_0
- s->updated_server_version = HostDBApplicationInfo::HTTP_VERSION_10;
- } else {
- // dont update the hostdb. let us try again with what we currently think.
- }
- } else if (s->hdr_info.server_response.version_get() == HTTPVersion(1, 1)) {
- if (s->current.server->http_version == HTTPVersion(0, 9)) {
- // update_hostdb_to_indicate_server_version_is_1_1
- s->updated_server_version = HostDBApplicationInfo::HTTP_VERSION_11;
- } else if (s->current.server->http_version == HTTPVersion(1, 0)) {
- // update_hostdb_to_indicate_server_version_is_1_1
- s->updated_server_version = HostDBApplicationInfo::HTTP_VERSION_11;
- } else {
- // dont update the hostdb. let us try again with what we currently think.
- }
- } else {
- // dont update the hostdb. let us try again with what we currently think.
+ HostDBApplicationInfo::HttpVersion real_version =
s->state_machine->server_txn->get_version(s->hdr_info.server_response);
+ if (real_version != s->host_db_info.app.http_data.http_version) {
+ TxnDebug("http_trans", "Update hostdb history of server HTTP version
0x%x", real_version);
+ // Need to update the hostdb
+ s->updated_server_version = real_version;
Review comment:
Seems like we can do this without H2 to Origin.
##########
File path: proxy/http/HttpSessionManager.cc
##########
@@ -190,9 +196,11 @@ ServerSessionPool::acquireSession(sockaddr const *addr,
CryptoHash const &hostna
}
if (zret == HSM_DONE) {
to_return = first;
- HTTP_DECREMENT_DYN_STAT(http_pooled_server_connections_stat);
- m_ip_pool.erase(first);
- m_fqdn_pool.erase(to_return);
+ if (!to_return->is_multiplexing()) {
+ HTTP_DECREMENT_DYN_STAT(http_pooled_server_connections_stat);
+ m_ip_pool.erase(first);
+ m_fqdn_pool.erase(to_return);
Review comment:
Why don't you call removeSession? It seems like removeSession just does
these three things with logging.
I know H2 session calls removeSession directly somehow, but I think we could
add removeSession separately for current code.
##########
File path: proxy/http/HttpSessionManager.h
##########
@@ -110,7 +113,7 @@ class HttpSessionManager
public:
HttpSessionManager() {}
~HttpSessionManager() {}
- HSMresult_t acquire_session(Continuation *cont, sockaddr const *addr, const
char *hostname, ProxyTransaction *ua_txn, HttpSM *sm);
+ HSMresult_t acquire_session(HttpSM *cont, sockaddr const *addr, const char
*hostname, ProxyTransaction *ua_txn);
Review comment:
Seems like a general cleanup.
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -1826,10 +1830,14 @@ HttpTransact::ReDNSRoundRobin(State *s)
s->next_action = how_to_open_connection(s);
} else {
// Our ReDNS failed so output the DNS failure error message
- build_error_response(s, HTTP_STATUS_BAD_GATEWAY, "Cannot find server.",
"connect#dns_failed");
+ // Set to internal server error so later logging will pick up
SQUID_LOG_ERR_DNS_FAIL
Review comment:
Not 100% sure if we can do this separately, but it doesn't seem like a
change for H2 to Origin.
##########
File path: proxy/http2/HTTP2.cc
##########
@@ -729,7 +766,7 @@ http2_decode_header_blocks(HTTPHdr *hdr, const uint8_t
*buf_start, const uint32_
if (hdr->field_find(MIME_FIELD_CONNECTION, MIME_LEN_CONNECTION) != nullptr ||
hdr->field_find(MIME_FIELD_KEEP_ALIVE, MIME_LEN_KEEP_ALIVE) != nullptr ||
hdr->field_find(MIME_FIELD_PROXY_CONNECTION, MIME_LEN_PROXY_CONNECTION)
!= nullptr ||
- hdr->field_find(MIME_FIELD_TRANSFER_ENCODING,
MIME_LEN_TRANSFER_ENCODING) != nullptr ||
+ // hdr->field_find(MIME_FIELD_TRANSFER_ENCODING,
MIME_LEN_TRANSFER_ENCODING) != nullptr ||
Review comment:
Assuming this is related to gRPC. If normal H2 requests work fine with
this line, I'd suggest making this change separately after this big change.
That would allow us to back out only gRPC support but not entire H2 to Origin
support.
##########
File path: proxy/http/HttpTransact.cc
##########
@@ -3704,26 +3717,16 @@ HttpTransact::handle_response_from_server(State *s)
case OUTBOUND_CONGESTION:
TxnDebug("http_trans", "[handle_response_from_server] Error. congestion
control -- congested.");
SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_FAILURE);
- s->current.server->set_connect_fail(EUSERS); // too many users
+ s->set_connect_fail(EUSERS); // too many users
handle_server_connection_not_open(s);
break;
case OPEN_RAW_ERROR:
- /* fall through */
Review comment:
I don't have strong opinion on this, but some may prefer to have
`[[fallthrough]];`, which is available since C++17.
In any cases, we could do this separately.
##########
File path: proxy/http/HttpTunnel.cc
##########
@@ -1161,6 +1165,8 @@ HttpTunnel::producer_handler(int event,
HttpTunnelProducer *p)
// If the write completes on the stack (as it can for http2), then
// consumer could have called back by this point. Must treat this as
// a regular read complete (falling through to the following cases).
+ p->bytes_read = p->init_bytes_done;
+ // FALLTHROUGH
Review comment:
Nitpick: If I feel like this comment is needed, I use `[[fallthrough]];`.
##########
File path: src/tscore/BufferWriterFormat.cc
##########
@@ -896,8 +897,12 @@ bwformat(BufferWriter &w, BWFSpec const &spec, bwf::Errno
const &e)
if (spec.has_numeric_type()) { // if numeric type, print just
the numeric part.
w.print(number_fmt, e._e);
} else {
- w.write(short_name(e._e));
- w.write(strerror(e._e));
+ if (e._e < 0) {
Review comment:
Definitely not a part of H2 to Origin.
##########
File path: tests/gold_tests/redirect/redirect_actions.test.py
##########
@@ -184,7 +184,7 @@ class ActionE(Enum):
Follow = {'config': 'follow', 'expectedStatusLine': 'HTTP/1.1 204 No
Content\r\n'}
# Added to test failure modes.
- Break = {'expectedStatusLine': 'HTTP/1.1 502 Cannot find server.\r\n'}
+ Break = {'expectedStatusLine': 'HTTP/1.1 500 Cannot find server.\r\n'}
Review comment:
Is this related to H2 to Origin?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
