Copilot commented on code in PR #13820:
URL: https://github.com/apache/trafficserver/pull/13820#discussion_r4249237419
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -6945,10 +6947,12 @@ HttpTransact::is_request_retryable(State *s)
return false;
}
- // If safe requests are retryable, it should be safe to retry safe requests
irrespective of bytes sent or connection state
- // according to RFC the following methods are safe
(https://tools.ietf.org/html/rfc7231#section-4.2.1)
+ // Safe methods (RFC 9110, section 9.2.1) can be retried irrespective of
bytes sent or connection state. QUERY
+ // (draft-ietf-httpbis-safe-method-w-body) is safe too, but its content is
not kept for replay on this path, so once
+ // any of its request has been sent it is treated like an unsafe method,
whether or not the content itself went out. A
+ // buffered QUERY can still be retried when the origin reports it was not
processed, see HttpSM::prepare_for_origin_retry.
// Otherwise, if there was no error establishing the connection (and we sent
bytes)-- we cannot retry
- if (!HttpTransactHeaders::is_method_safe(s->method) && s->current.state !=
CONNECTION_ERROR &&
+ if ((!HttpTransactHeaders::is_method_safe(s->method) || s->method ==
HTTP_WKSIDX_QUERY) && s->current.state != CONNECTION_ERROR &&
Review Comment:
The retry gating condition is correct but hard to read because it mixes the
general safe/unsafe rule with the `QUERY` exception inline. Consider
refactoring into a clearly named boolean (e.g.,
`treat_as_unsafe_after_sending_bytes`) or explicitly structuring it as
`(is_safe && method != QUERY)` vs `!is_safe` to make the special-case behavior
self-evident and reduce the chance of future regressions when additional safe
methods are added.
##########
src/proxy/http/HttpTransact.cc:
##########
@@ -6945,10 +6947,12 @@ HttpTransact::is_request_retryable(State *s)
return false;
}
- // If safe requests are retryable, it should be safe to retry safe requests
irrespective of bytes sent or connection state
- // according to RFC the following methods are safe
(https://tools.ietf.org/html/rfc7231#section-4.2.1)
+ // Safe methods (RFC 9110, section 9.2.1) can be retried irrespective of
bytes sent or connection state. QUERY
+ // (draft-ietf-httpbis-safe-method-w-body) is safe too, but its content is
not kept for replay on this path, so once
+ // any of its request has been sent it is treated like an unsafe method,
whether or not the content itself went out. A
Review Comment:
The comment has a grammatical issue: 'any of its request' reads incorrectly.
Reword to something like 'once any part of its request has been sent' (or 'once
any of its request bytes have been sent') to match the intent.
##########
src/proxy/http/HttpTransactHeaders.cc:
##########
@@ -125,9 +127,9 @@
HttpTransactHeaders::insert_supported_methods_in_response(HTTPHdr *response, int
const char *methods[] = {
HTTP_METHOD_CONNECT.c_str(), HTTP_METHOD_DELETE.c_str(),
HTTP_METHOD_GET.c_str(), HTTP_METHOD_HEAD.c_str(),
HTTP_METHOD_OPTIONS.c_str(), HTTP_METHOD_POST.c_str(),
HTTP_METHOD_PURGE.c_str(), HTTP_METHOD_PUT.c_str(),
- HTTP_METHOD_PUSH.c_str(), HTTP_METHOD_TRACE.c_str(),
+ HTTP_METHOD_PUSH.c_str(), HTTP_METHOD_QUERY.c_str(),
HTTP_METHOD_TRACE.c_str(),
};
- char inline_buffer[64];
+ char inline_buffer[128];
Review Comment:
Bumping `inline_buffer` from 64 to 128 addresses the immediate size issue,
but it introduces another arbitrary limit. If feasible in this function, prefer
computing the required buffer size from the method strings (and any existing
`Allow` value being appended to) and allocating when needed, so future method
additions don't require another manual bump.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
