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

Reply via email to