[GitHub] [trafficserver] cmcfarlen commented on a change in pull request #8591: add proxy.config.http.cache.max_open_write_retry_timeout parameter

[GitBox <gi...@apache.org>]

cmcfarlen commented on a change in pull request #8591:
URL: https://github.com/apache/trafficserver/pull/8591#discussion_r839953545



##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -139,18 +139,20 @@ HttpCacheSM::state_cache_open_read(int event, void *data)
     }
     break;
 
-  case EVENT_INTERVAL:
+  case EVENT_INTERVAL: {
     // Retry the cache open read if the number retries is less
     // than or equal to the max number of open read retries,
     // else treat as a cache miss.
-    ink_assert(open_read_tries <= master_sm->t_state.txn_conf->max_cache_open_read_retries || write_locked);
+    bool const retries_done = (master_sm->t_state.txn_conf->max_cache_open_read_retries < open_read_tries);
+    ink_assert(retries_done || write_locked);

Review comment:
       Maybe the read retry count is checked and handled somewhere else.  In that case, disregard the second part of the comment above.

##########
File path: proxy/http/HttpCacheSM.cc
##########
@@ -139,18 +139,20 @@ HttpCacheSM::state_cache_open_read(int event, void *data)
     }
     break;
 
-  case EVENT_INTERVAL:
+  case EVENT_INTERVAL: {
     // Retry the cache open read if the number retries is less
     // than or equal to the max number of open read retries,
     // else treat as a cache miss.
-    ink_assert(open_read_tries <= master_sm->t_state.txn_conf->max_cache_open_read_retries || write_locked);
+    bool const retries_done = (master_sm->t_state.txn_conf->max_cache_open_read_retries < open_read_tries);
+    ink_assert(retries_done || write_locked);

Review comment:
       This logic looks wrong to me.  If the max is 4 and open_read_tries is 1, then retries_done will be false (4 is not less than 1).  If write_locked is false, then this will be `false || false` which will trigger the assert.  Perhaps this should be `ink_assert(!retries_done || write_locked)`?
   
   Further, I see this assert, but I don't see that the call to `do_cache_open_read` is ever skipped.  In debug mode, going past the max will abort the process and in release mode it will always retry.  Perhaps this should go into an error state after retries_done is true?
   




-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org