[GitHub] [trafficserver] sudheerv commented on pull request #7226: Bugfix: set a default inactivity timeout only if a read or write I/O operation was set

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

sudheerv commented on pull request #7226:
URL: https://github.com/apache/trafficserver/pull/7226#issuecomment-700858725


   The change does sound functionally correct, although, I wonder what happens when active timeout isn't configured (ie set to "0"). It'd leave the VC unmonitored because, there's neither the "active", nor the "inactive" timeout on it now. 
   
   That said, we also generally are trying to remove the default inactive timeout mechanism entirely (or set it to a super high value - e.g hours or even days as opposed to the current 5 min value that we are forced to set in production), which means we have to make sure we find all corner cases where there's no specific timeout is monitoring a VC. I think the DNS lookup use case you raised is a nice example of such scenario - as long as we enforce a timeout on the SM for DNS lookup and clean up the VC associated on a timeout, we should be good, but, I'm not sure if there are more such scenarios.
   
   Coming back to the specific change in the PR, it might be worth changing it to 
   
   
   {code}
         if (ne->next_inactivity_timeout_at == 0 && nh.config.default_inactivity_timeout > 0 &&
             (ne->read.enabled || ne->write.enabled || ne->active_timeout_in == 0) {
   {code}


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

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