You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/07/19 18:22:49 UTC
[GitHub] [trafficserver] shinrich opened a new pull request #8083: Cancel active timeout when releasing session
shinrich opened a new pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083
More details are in the issue. This fix cancels the active timeout when the server session is released. It may be a while still before the HttpSM is killed and the server session is actually put in the server session pool as discussed in the issue.
This PR also partially undoes the change in PR #7618. I actually reviewed and approved that one, but on looking at it more closely while debugging this issue, I think the change is faulty since it will lose any overriden value associated with the Transaction, making the overridable nature of the keep alive timeout ineffective.
This change fixed our TxnBox-based reproducing test. I am getting ready to set this up for testing in our 9.1 based production build.
I'll try to set up a reproducing test in another PR that uses a transform plugin that is already in the trafficserver code base.
This closes #8079
--
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
[GitHub] [trafficserver] shinrich commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-889173435
Right. Before your change, the code was setting C back to C. I assume in an attempt to ensure the full inactivity timeout was applied during the keep-alive period. But before calling ServerSessionPool::releaseSession, the caller had set the inactivity timeout to B (the override version of the keepalive timeout).
Unfortunately, by the time we get to ServerSessionPool::releaseSession, the code does not have direct access to B. No access to the state machine. We could update the API, so B is passed along the release call chain, so it can be set in ServerSessionPool::releaseSession.
Ultimately, the keep-alive timeout should really be overridable at the session level. But unfortunately, our existing override mechanism is centered around the HttpSM. We have talked in various meetings about making session more first class objects, but I'm afraid that is a bigger decision effort that this PR.
--
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
[GitHub] [trafficserver] maskit commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-886303148
And like I said on #7618, a session could be kept in the pool forever if inactive timeout was set to 0.
--
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
[GitHub] [trafficserver] bryancall commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-882922501
@vmamidi is going to take a look
--
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
[GitHub] [trafficserver] bryancall commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-882922501
@vmamidi is going to take a look
--
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
[GitHub] [trafficserver] maskit commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-886301215
I don't understand why overridden timeout values have to be retained when a session is returned to the session pool. Retaining the values means you will acquire a session that has unpredictable timeout values which depend on previous transactions.
--
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
[GitHub] [trafficserver] shinrich commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-886910969
The inactivity timeout is set before returning to the pool. Within the return to pool logic, we could check if the inactivity timeout is 0 and set it from the global setting. This would protect us from future errors where the caller does not set the inactivity timeout.
--
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
[GitHub] [trafficserver] shinrich merged pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
shinrich merged pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083
--
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
[GitHub] [trafficserver] shinrich commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-886887049
This issue is for proxy.config.http.keep_alive_no_activity_timeout_out. The documentation identifies this setting as overridable. So we need to set the timeout to that transaction-specific value before it is added to the session pool. Otherwise the setting is effectively not overrideable, and we should update the documentation.
https://docs.trafficserver.apache.org/en/latest/admin-guide/files/records.config.en.html?highlight=keepalive#proxy-config-http-keep-alive-no-activity-timeout-out
--
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
[GitHub] [trafficserver] maskit commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-887296815
I guess this confusion comes from having multiple values for the timeout. We have a) the original value in records.config, b) new value set by a transaction, and c) the effective value in a session instance. And c) can be set to 0 if you call cancel_timeout() or something.
Before my change on #7618, the code seemed like to reset c) with c). It didn't make sense to me, so I changed the code to reset c) with a). If we want to override the timeout, what we really need to do is probably to reset c) with b). Does it make sense?
--
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
[GitHub] [trafficserver] bryancall commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-882922501
@vmamidi is going to take a look
--
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
[GitHub] [trafficserver] maskit commented on pull request #8083: Cancel active timeout when releasing session
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8083:
URL: https://github.com/apache/trafficserver/pull/8083#issuecomment-889550737
Fair enough. I still think resetting the value with C is incorrect, but resetting the value with A is also incorrect and sounds like C has a bit more practical value.
--
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