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.
---
