[GitHub] [trafficserver] maskit commented on pull request #7618: Reload server session inactivity timeout before placing a session into the pool

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

maskit commented on pull request #7618:
URL: https://github.com/apache/trafficserver/pull/7618#issuecomment-806386365


   I can see the pattern in HttpSM, but those are not the only places calling `Http1ServerSession::release()`. It's called from Http1ClientSession as well and the timeout is not reset there. 
   ```
   $ git grep -E -- "->release\([^\)]"
   plugins/prefetch/fetch.cc:    ret &= _policy->release(url);
   plugins/prefetch/fetch.cc:  permitted     = _unique->release(url);
   plugins/prefetch/fetch.cc:      _state->release(_cachekey);
   plugins/prefetch/fetch.cc:      _state->release(cachekey);
   plugins/prefetch/plugin.cc:            state->release(data->_cachekey);
   plugins/prefetch/plugin.cc:          state->release(data->_cachekey);
   proxy/http/Http1ClientSession.cc:    bound_ss->release(nullptr);
   proxy/http/Http1ClientSession.cc:    bound_ss->release(nullptr);
   proxy/http/Http1ClientSession.cc:  this->release(&trans);
   proxy/http/HttpSM.cc:      server_session->release(nullptr);
   proxy/http/HttpSM.cc:    ua_txn->release(ua_buffer_reader);
   proxy/http/HttpSM.cc:    ua_txn->get_proxy_ssn()->release(ua_txn);
   proxy/http/HttpSM.cc:        existing_ss->release(nullptr);
   proxy/http/HttpSM.cc:      existing_ss->release(nullptr);
   proxy/http/HttpSM.cc:      server_session->release(nullptr);
   proxy/http/HttpSessionManager.cc:    to_return->release(nullptr);
   ```
   
   > I'd suggest either, adding an ink_release_assert in ServerSessionPool::releaseSession to verify that the timeout is currently set to a non-zero value. And/or pass along the keep_alive timeout value as an argument to release, so they are done together always.
   
   I don't think those are great idea. I could add ink_release_assert to check whether it's really because of setting timeout to 0, but that would be a disaster if it is. And passing timeout value to the release function doesn't guarantee anything because you can still pass 0.
   
   IMO, ServerSessionPool or HttpSessionManager should be responsible for setting a valid timeout before placing server sessions into the pool. How server sessions are managed in the pool is none of HTTP state machine's business. We could have another timeout for sessions in the pool in addition to inactivity timeout for sessions in use. I think requiring session pool users to set correct values before returning a session is too much and it's unreliable.


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