Github user zwoop commented on a diff in the pull request: https://github.com/apache/trafficserver/pull/1404#discussion_r99178025 --- Diff: proxy/http/HttpTransact.cc --- @@ -6487,9 +6487,15 @@ HttpTransact::is_request_valid(State *s, HTTPHdr *incoming_request) bool HttpTransact::is_request_retryable(State *s) { + // 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) // If there was no error establishing the connection (and we sent bytes)-- we cannot retry --- End diff -- So, not 100% sure, but the original code did this: ``` // If the connection was established-- we cannot retry if (s->state_machine->server_request_hdr_bytes > 0) { ``` Meaning, it would only retry if server_request_hdr_bytes is > 0. I'm not sure what exactly that catches, but it seemed to imply that it'd only retry as long as it wasn't established? But what does that mean, and what's the relation to server_request_hdr_bytes? The reason I'm asking now is that it's returnable regardless of these "states", right? All it's checking is that the method is safe, and the configuration allows retrying safe tests? What I'd like to see is that we get back to the original behavior as much as possible, with the addition of checking for safe methods before retrying. Finally, I wonder if it'd make sense to create a isMethodSafe() function somewhere? For example, I see similar checks in ``` // draft-stenberg-httpbis-tcp recommends only enabling TFO on safe, indempotent methods or // those with intervening protocol layers (eg. TLS). if (scheme_to_use == URL_WKSIDX_HTTPS || t_state.method == HTTP_WKSIDX_CONNECT || t_state.method == HTTP_WKSIDX_DELETE || t_state.method == HTTP_WKSIDX_GET || t_state.method == HTTP_WKSIDX_HEAD || t_state.method == HTTP_WKSIDX_PUT) { opt.f_tcp_fastopen = (t_state.txn_conf->sock_option_flag_out & NetVCOptions::SOCK_OPT_TCP_FAST_OPEN); } ``` which makes me think this should be a function? Particularly since this looks wrong, PUT isn't safe. Also note that the comment above is wrong, if you change this code, it should be "idempotent" here as well :). And afaik, all safe methods are also idempotent, no? I don't know if we check for safe methods elsewhere, but worth to take a look. If I had a choice, I'd probably create two methods just to get this properly documented in code: ```C bool isMethodSafe(int method); bool isMethodIdempotent(int method); ``` And link to the RFC 7231 as well here instead.
--- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---