[GitHub] [trafficserver] sudheerv commented on pull request #7134: Do not lose original inactivity timeout on disable

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

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


   @shinrich The change in your PR looks reasonable to me. This is how the code looked in 6.2.x as well.
   
   As you said, Inactivity cop's take over of inactive timeout when no timeout is set is by design, and we recently tried to analyze why we'd even have situations where such guard/catch-all is required. I think we sort of found most (probably all such) cases, one example was a missing SSL Handshake timeout and confirmed by adding explicit metrics to see how often is the Inactivity Cop tries to take over the inactive timeout with default timeout, and of those, how often it actually _applies_ the default timeout. We found that the former is significant but the latter is 0 in prod after fixing some of the gaps (example below from a prod box). 
   
   {code}
   [svinukon@ltx1-app42432 i001]$ sudo ./chroot-exec traffic_ctl  metric match  default
   proxy.process.net.default_inactivity_timeout_applied 31593
   proxy.process.net.default_inactivity_timeout_count 0
   {code}
   
   Also, I think the logic in inactivity cop taking over the VC's inactivity by setting default timeout is not actually introduced by PR 6755. PR 6755 merely did some refactoring to be able to detect default vs non-default inactivity timeout being set. The takeover logic has always been there AFAIR (at least, from 5.x?). I think @bryancall added this guard because we were seeing lots of connection leaks without that back in 5.x.


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