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 2022/12/20 20:12:17 UTC
[GitHub] [trafficserver] bneradt opened a new pull request, #9268: Make net.default_inactivity_timeout overridable
bneradt opened a new pull request, #9268:
URL: https://github.com/apache/trafficserver/pull/9268
This makes the default_inactivity_timeout configuration an overridable configuration.
--
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] serrislew commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
serrislew commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1066481193
##########
iocore/net/UnixNet.cc:
##########
@@ -70,20 +71,30 @@ class InactivityCop : public Continuation
continue;
}
+ if (ne->default_inactivity_timeout_in == -1) {
+ // If no context-specific default inactivity timeout has been set by an
+ // override plugin, then use the global default.
+ Debug("inactivity_cop",
+ "vc: %p setting a the global default inactivity timeout of %d, next_inactivity_timeout_at: %" PRId64, ne,
+ nh.config.default_inactivity_timeout, ne->next_inactivity_timeout_at);
+ ne->set_default_inactivity_timeout(HRTIME_SECONDS(nh.config.default_inactivity_timeout));
Review Comment:
Not sure when `initialize_thread_for_net` is called relative to when configs get changed but why don't we use `global_config` var, which reads the config values as opposed to `nh->config` which is just a copy that can possibly have stale data? Is it guaranteed to be updated when configs change?
--
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] bneradt commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1083338363
##########
proxy/ProxySession.cc:
##########
@@ -227,6 +227,14 @@ ProxySession::set_inactivity_timeout(ink_hrtime timeout_in)
}
}
+void
+ProxySession::set_default_inactivity_timeout(ink_hrtime timeout_in)
+{
+ if (_vc) {
+ _vc->set_default_inactivity_timeout(timeout_in);
Review Comment:
Oh and I should add, in case someone reads this in the future, actually "setting" things to use the default inactivity timeout is done via the cancel_inactivity_timeout function. That cancels the explicit inactivity timeout, which effectively leads to falling back on the default 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] bneradt commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1071526549
##########
iocore/net/UnixNet.cc:
##########
@@ -70,20 +71,30 @@ class InactivityCop : public Continuation
continue;
}
+ if (ne->default_inactivity_timeout_in == -1) {
+ // If no context-specific default inactivity timeout has been set by an
+ // override plugin, then use the global default.
+ Debug("inactivity_cop",
+ "vc: %p setting a the global default inactivity timeout of %d, next_inactivity_timeout_at: %" PRId64, ne,
Review Comment:
Good catch.
--
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] serrislew commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
serrislew commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1066489125
##########
proxy/ProxySession.cc:
##########
@@ -227,6 +227,14 @@ ProxySession::set_inactivity_timeout(ink_hrtime timeout_in)
}
}
+void
+ProxySession::set_default_inactivity_timeout(ink_hrtime timeout_in)
+{
+ if (_vc) {
+ _vc->set_default_inactivity_timeout(timeout_in);
Review Comment:
to clarify, when _vc is of type UnixNetVConnection, why would we not want `next_inactivity_timeout_at` to be updated as that is no longer part of the function?
--
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] bneradt commented on pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#issuecomment-1363096170
[approve ci]
--
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] serrislew commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
serrislew commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1066479034
##########
iocore/net/UnixNet.cc:
##########
@@ -70,20 +71,30 @@ class InactivityCop : public Continuation
continue;
}
+ if (ne->default_inactivity_timeout_in == -1) {
+ // If no context-specific default inactivity timeout has been set by an
+ // override plugin, then use the global default.
+ Debug("inactivity_cop",
+ "vc: %p setting a the global default inactivity timeout of %d, next_inactivity_timeout_at: %" PRId64, ne,
Review Comment:
this is nit-picky but I think you can remove the "a"
--
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] serrislew commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
serrislew commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1066489125
##########
proxy/ProxySession.cc:
##########
@@ -227,6 +227,14 @@ ProxySession::set_inactivity_timeout(ink_hrtime timeout_in)
}
}
+void
+ProxySession::set_default_inactivity_timeout(ink_hrtime timeout_in)
+{
+ if (_vc) {
+ _vc->set_default_inactivity_timeout(timeout_in);
Review Comment:
to confirm, if _vc is of type UnixNetVConnection, would we not want it to update `next_inactivity_timeout_at` as that is no longer part of the function?
--
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] bneradt commented on pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#issuecomment-1397750864
[approve ci autest]
--
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] serrislew commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by "serrislew (via GitHub)" <gi...@apache.org>.
serrislew commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1082949895
##########
proxy/ProxySession.cc:
##########
@@ -227,6 +227,14 @@ ProxySession::set_inactivity_timeout(ink_hrtime timeout_in)
}
}
+void
+ProxySession::set_default_inactivity_timeout(ink_hrtime timeout_in)
+{
+ if (_vc) {
+ _vc->set_default_inactivity_timeout(timeout_in);
Review Comment:
I see, thanks for the explanation!
--
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] bneradt commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1081955533
##########
proxy/ProxySession.cc:
##########
@@ -227,6 +227,14 @@ ProxySession::set_inactivity_timeout(ink_hrtime timeout_in)
}
}
+void
+ProxySession::set_default_inactivity_timeout(ink_hrtime timeout_in)
+{
+ if (_vc) {
+ _vc->set_default_inactivity_timeout(timeout_in);
Review Comment:
This is a good question.
Note that before this patch, `set_default_inactivity_timeout` was called exactly once in UnixNet.cc. That was called off of NetEvent objects which were only ever `UnixNetVConnection` or `QUICNetVConnection` concrete types, both of which took the `UnixNetVConnection::set_default_inactivity_timeout` definition. In the previous use of this function, `set_default_inactivity_timeout` meant: (1) cancel any current activity_timeout_in, (2) set the next_inactivity_timeout_at to the provided default timeout value, and (3) set a boolean indicating that the current inactivity timeout is a default one. In other words, `set_default_inactivity_timeout` didn't mean "set the configured default inactivity timeout to this new value", it meant, "make the next timeout the provided value and indicate that it was set from the default inactivity timeout value". It was an ad-hoc implementation for the inactivity cop's single use of it.
This patch changes the meaning of the function to something more simple: "set the configured default_inactivity_timeout value to this new value". And now it is only called in:
1. In HttpTransact to provide a transaction level override.
2. In InactivityCop if no override has been set to set the global value.
So, to answer your question directly, we no longer set `next_inactivity_timeout_at` because the meaning of `set_default_inactivity_timeout` has changed to mean "set the configured default_inactivity_timeout", not the previous meaning of "configure an inactivity timeout value from this default 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
[GitHub] [trafficserver] serrislew commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
serrislew commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1066478726
##########
iocore/net/UnixNet.cc:
##########
@@ -70,20 +71,30 @@ class InactivityCop : public Continuation
continue;
}
+ if (ne->default_inactivity_timeout_in == -1) {
+ // If no context-specific default inactivity timeout has been set by an
+ // override plugin, then use the global default.
+ Debug("inactivity_cop",
+ "vc: %p setting a the global default inactivity timeout of %d, next_inactivity_timeout_at: %" PRId64, ne,
+ nh.config.default_inactivity_timeout, ne->next_inactivity_timeout_at);
+ ne->set_default_inactivity_timeout(HRTIME_SECONDS(nh.config.default_inactivity_timeout));
+ }
+
// set a default inactivity timeout if one is not set
// The event `EVENT_INACTIVITY_TIMEOUT` only be triggered if a read
// or write I/O operation was set by `do_io_read()` or `do_io_write()`.
- if (ne->next_inactivity_timeout_at == 0 && nh.config.default_inactivity_timeout > 0 &&
- (ne->read.enabled || ne->write.enabled)) {
+ if (ne->next_inactivity_timeout_at == 0 && ne->default_inactivity_timeout_in > 0 && (ne->read.enabled || ne->write.enabled)) {
Debug("inactivity_cop", "vc: %p inactivity timeout not set, setting a default of %d", ne,
nh.config.default_inactivity_timeout);
- ne->set_default_inactivity_timeout(HRTIME_SECONDS(nh.config.default_inactivity_timeout));
+ ne->use_default_inactivity_timeout = true;
Review Comment:
Don't we also want to set `inactivity_timeout_in = 0` since that gets removed from the `set_default_inactivity_timeout` function and needed to return true for `is_default_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] bneradt commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1071623221
##########
iocore/net/UnixNet.cc:
##########
@@ -70,20 +71,30 @@ class InactivityCop : public Continuation
continue;
}
+ if (ne->default_inactivity_timeout_in == -1) {
+ // If no context-specific default inactivity timeout has been set by an
+ // override plugin, then use the global default.
+ Debug("inactivity_cop",
+ "vc: %p setting a the global default inactivity timeout of %d, next_inactivity_timeout_at: %" PRId64, ne,
+ nh.config.default_inactivity_timeout, ne->next_inactivity_timeout_at);
+ ne->set_default_inactivity_timeout(HRTIME_SECONDS(nh.config.default_inactivity_timeout));
+ }
+
// set a default inactivity timeout if one is not set
// The event `EVENT_INACTIVITY_TIMEOUT` only be triggered if a read
// or write I/O operation was set by `do_io_read()` or `do_io_write()`.
- if (ne->next_inactivity_timeout_at == 0 && nh.config.default_inactivity_timeout > 0 &&
- (ne->read.enabled || ne->write.enabled)) {
+ if (ne->next_inactivity_timeout_at == 0 && ne->default_inactivity_timeout_in > 0 && (ne->read.enabled || ne->write.enabled)) {
Debug("inactivity_cop", "vc: %p inactivity timeout not set, setting a default of %d", ne,
nh.config.default_inactivity_timeout);
- ne->set_default_inactivity_timeout(HRTIME_SECONDS(nh.config.default_inactivity_timeout));
+ ne->use_default_inactivity_timeout = true;
Review Comment:
Yes, good catch. Missing that didn't cause a functional timeout issue, but would make it so the stat wasn't incremented, which I hadn't noticed.
--
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] bneradt commented on a diff in pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#discussion_r1081903502
##########
iocore/net/UnixNet.cc:
##########
@@ -70,20 +71,30 @@ class InactivityCop : public Continuation
continue;
}
+ if (ne->default_inactivity_timeout_in == -1) {
+ // If no context-specific default inactivity timeout has been set by an
+ // override plugin, then use the global default.
+ Debug("inactivity_cop",
+ "vc: %p setting a the global default inactivity timeout of %d, next_inactivity_timeout_at: %" PRId64, ne,
+ nh.config.default_inactivity_timeout, ne->next_inactivity_timeout_at);
+ ne->set_default_inactivity_timeout(HRTIME_SECONDS(nh.config.default_inactivity_timeout));
Review Comment:
That's an interesting idea, but I think we better stick with reading the config from the nethandler here rather than from global_config. Currently global_config is populated into each handler with each handler registering the `update_nethandler_config` for updates that I presume gets processed in a timely fashion. All the configurations are looked at in this file from nethandler, not directly from global_config. That is, notice that global_config is only used for nethandler config initialization and then subsequent updates via the update handler, it's never used to access the runtime values. I'm not sure, but this may be due to threading coordination reasons.
In any case, I'd say we stick with what is done in the rest of this file for this PR. If we want to look into changing things in UnixNet.cc to access global_config directly, let's do that in a separate 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] bneradt commented on pull request #9268: Make net.default_inactivity_timeout overridable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268#issuecomment-1397718806
[approve ci fedora]
--
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] bneradt merged pull request #9268: Make net.default_inactivity_timeout overridable
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #9268:
URL: https://github.com/apache/trafficserver/pull/9268
--
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