You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "moonchen (via GitHub)" <gi...@apache.org> on 2026/08/04 19:17:06 UTC
[PR] Destroy replaced configs on ET_TASK [trafficserver]
moonchen opened a new pull request, #13491:
URL: https://github.com/apache/trafficserver/pull/13491
The deferred destruction of a replaced config runs on a network thread today. `ConfigProcessor::set()` schedules it with `schedule_in()`, which defaults to ET_CALL, so 60 seconds later an ET_NET thread runs the destructor inside the drain phase of its event loop and is blocked for as long as the config takes to release, which is bounded only by the size of the config.
Move that work to ET_TASK. `ConfigProcessor::release()` is the only place a config is destroyed, but two callers reach it: the releaser at 60 seconds, which destroys the config whenever nothing else still holds a reference, and a transaction that outlived the releaser and drops the last reference itself. This schedules the releaser on ET_TASK and hands the destructor from the transaction path to ET_TASK as well, so neither can block a network thread.
The 60 second wait is unchanged. Shortening it would narrow the window that makes the load-then-increment in `ConfigProcessor::get()` safe, so it should not be tuned just because the destructor moved.
The `config` debug tag now reports the duration of each destruction and the thread that ran it.
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen merged PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3715597646
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
Review Comment:
If `ink_get_thread_name()` fails or does not guarantee null-termination in all cases, the debug log may print uninitialized/garbage data. Consider zero-initializing `thread_name` (e.g., `thread_name[0] = '\\0';`) and/or checking the return value and falling back to a known string like `\"<unknown>\"`.
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
Review Comment:
The format string uses `%d` but `id` is an `unsigned int`. This is a varargs type mismatch and can cause undefined behavior. Use `%u` (or an appropriate `PRI*` macro) to match the unsigned type.
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(new_ProxyMutex()), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // Until the task threads are registered and spawned, ET_TASK is ET_CALL and there is nowhere else
+ // to send this.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK) || eventProcessor.thread_group[ET_TASK]._count == 0) {
+ return false;
+ }
+
+ ConfigInfoDestroyer *destroyer = new ConfigInfoDestroyer(id, info);
+
+ if (eventProcessor.schedule_imm(destroyer, ET_TASK) == nullptr) {
+ // The event system is shutting down and will never run the destroyer.
+ delete destroyer;
+ return false;
+ }
+
+ return true;
}
+} // namespace
+
class ConfigInfoReleaser : public Continuation
{
Review Comment:
Closing the anonymous namespace here changes the linkage/visibility of `ConfigInfoReleaser` (and anything that follows) from internal to external, which can unintentionally export symbols and diverge from the prior encapsulation. If `ConfigInfoReleaser` is intended to remain TU-local, keep it inside the unnamed namespace (move this closing brace below it or remove the early close).
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(new_ProxyMutex()), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // Until the task threads are registered and spawned, ET_TASK is ET_CALL and there is nowhere else
+ // to send this.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK) || eventProcessor.thread_group[ET_TASK]._count == 0) {
+ return false;
+ }
Review Comment:
This reaches into `eventProcessor.thread_group[ET_TASK]._count` (a member named like an internal field). If there’s an existing public API/helper to query whether an event type has threads (or whether `ET_TASK` is available), prefer that over inspecting `_count` directly to reduce coupling to `eventProcessor` internals.
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3716404906
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(new_ProxyMutex()), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // Until the task threads are registered and spawned, ET_TASK is ET_CALL and there is nowhere else
+ // to send this.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK) || eventProcessor.thread_group[ET_TASK]._count == 0) {
+ return false;
+ }
+
+ ConfigInfoDestroyer *destroyer = new ConfigInfoDestroyer(id, info);
+
+ if (eventProcessor.schedule_imm(destroyer, ET_TASK) == nullptr) {
+ // The event system is shutting down and will never run the destroyer.
+ delete destroyer;
+ return false;
+ }
+
+ return true;
}
+} // namespace
+
class ConfigInfoReleaser : public Continuation
{
Review Comment:
`ConfigInfoReleaser` is already outside the unnamed namespace on master, so its linkage is unchanged here:
```c
namespace
{
DbgCtl dbg_ctl_config{"config"};
}
class ConfigInfoReleaser : public Continuation
```
This change moves that closing brace later rather than earlier, so the namespace now encloses strictly more than it did before. The diff reads as though a brace were added because the last new line lands immediately above the pre-existing one, which now closes a function instead of the namespace.
Moving `ConfigInfoReleaser` into the unnamed namespace would be a reasonable cleanup, but it is pre-existing and unrelated to this 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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#issuecomment-5232776673
Cherry-picked to the 10.2.x branch as 745b2e7e1ff81241521549298097c2b42061e460 for the 10.2.0 release.
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3716395107
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
Review Comment:
Fixed, changed to `%u`.
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
Review Comment:
Fixed, the buffer is now zero-initialised.
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3716393270
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH];
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %d in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(new_ProxyMutex()), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // Until the task threads are registered and spawned, ET_TASK is ET_CALL and there is nowhere else
+ // to send this.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK) || eventProcessor.thread_group[ET_TASK]._count == 0) {
+ return false;
+ }
Review Comment:
This is a crash check rather than a style choice. Between `TasksProcessor::register_event_type()` and `TasksProcessor::start()`, ET_TASK is a distinct event type whose thread group is still empty, so `assign_thread()` returns `thread_group[ET_TASK]._thread[0]`, which is null, and `EventProcessor::schedule()` dereferences it when it enqueues. There is no error return for that case.
`_count` is also the right field to read rather than a convenient proxy: `spawn_event_threads()` populates `_thread[]` before assigning `_count`, so `_count > 0` implies every pointer in the group is valid. The public accessor `active_group_threads()` exposes only `begin()` and `end()`, so testing it is less clear at the call site.
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3722909709
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH] = {};
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %u in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(nullptr), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // ET_TASK is ET_CALL until the task threads are registered, so before that point an ET_NET caller
+ // destroys the config on its own thread.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK)) {
Review Comment:
If the code is running on ET_TASK, then this skips scheduling an event and causes a direct call to destroy instead.
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3723264876
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH] = {};
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %u in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(nullptr), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // ET_TASK is ET_CALL until the task threads are registered, so before that point an ET_NET caller
+ // destroys the config on its own thread.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK)) {
+ return false;
+ }
+
+ ConfigInfoDestroyer *destroyer = new ConfigInfoDestroyer(id, info);
+
+ if (eventProcessor.schedule_imm(destroyer, ET_TASK) == nullptr) {
Review Comment:
The comment says you avoid scheduling when `ET_TASK` is effectively `ET_CALL` (before task threads are registered), but the current condition doesn’t actually test for that state. If `schedule_imm(..., ET_TASK)` still routes to `ET_CALL`, this may run destruction on the current ET_NET thread (the regression you’re trying to avoid). Consider explicitly checking whether task threads are registered/available before scheduling to `ET_TASK`, and only scheduling when `ET_TASK` is backed by real task threads.
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3723364897
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH] = {};
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %u in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(nullptr), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // ET_TASK is ET_CALL until the task threads are registered, so before that point an ET_NET caller
+ // destroys the config on its own thread.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK)) {
+ return false;
+ }
+
+ ConfigInfoDestroyer *destroyer = new ConfigInfoDestroyer(id, info);
+
+ if (eventProcessor.schedule_imm(destroyer, ET_TASK) == nullptr) {
Review Comment:
If a `ET_TASK` thread doesn't exist, it's a last resort to schedule on `ET_CALL`. That's expected. I don't think the comment says otherwise.
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "moonchen (via GitHub)" <gi...@apache.org>.
moonchen commented on PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#issuecomment-5218588444
> Approving, although I do not think it is wise to lock this in as a requirement. I don't think this should have an AuTest. This is a performance optimization and we should be able to change our minds and do something else without waiting for a major release. I would argue that adding the AuTest here is an unofficial guarantee that users can rely on this behavior.
I understand your concern. I'm not trying to guarantee this behavior going forward with the autest. My goal with the test is to ensure that the code change does what it's supposed to. My understand about autest is that they're not set in stone, even within one major release, and they can be changed when the expected behavior of the code changes.
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#issuecomment-5192297669
[approve ci autest 1]
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3722748583
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH] = {};
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %u in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(nullptr), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // ET_TASK is ET_CALL until the task threads are registered, so before that point an ET_NET caller
+ // destroys the config on its own thread.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK)) {
Review Comment:
Should this be `ET_NET` instead of `ET_TASK`?
--
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
Re: [PR] Destroy replaced configs on ET_TASK [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #13491:
URL: https://github.com/apache/trafficserver/pull/13491#discussion_r3723663052
##########
src/iocore/eventsystem/ConfigProcessor.cc:
##########
@@ -34,8 +37,69 @@ namespace
DbgCtl dbg_ctl_config{"config"};
+void
+destroy_config(unsigned int id, ConfigInfo *info)
+{
+ ink_hrtime start = ink_get_hrtime();
+
+ delete info;
+
+ if (dbg_ctl_config.on()) {
+ char thread_name[MAX_THREAD_NAME_LENGTH] = {};
+
+ ink_get_thread_name(thread_name, sizeof(thread_name));
+ DbgPrint(dbg_ctl_config, "Destroyed config %u in %" PRId64 " ns on thread %s", id, ink_get_hrtime() - start, thread_name);
+ }
+}
+
+/// Runs the destructor of a detached ConfigInfo on ET_TASK.
+class ConfigInfoDestroyer : public Continuation
+{
+public:
+ ConfigInfoDestroyer(unsigned int id, ConfigInfo *info) : Continuation(nullptr), m_id(id), m_info(info)
+ {
+ SET_HANDLER(&ConfigInfoDestroyer::handle_event);
+ }
+
+ int
+ handle_event(int /* event ATS_UNUSED */, void * /* edata ATS_UNUSED */)
+ {
+ destroy_config(m_id, m_info);
+ delete this;
+ return EVENT_DONE;
+ }
+
+private:
+ unsigned int m_id;
+ ConfigInfo *m_info;
+};
+
+/// Hand a detached ConfigInfo to ET_TASK for destruction. Returns false when the caller has to
+/// destroy it itself.
+bool
+destroy_config_on_task_thread(unsigned int id, ConfigInfo *info)
+{
+ EThread *ethread = this_ethread();
+
+ // ET_TASK is ET_CALL until the task threads are registered, so before that point an ET_NET caller
+ // destroys the config on its own thread.
+ if (ethread == nullptr || ethread->is_event_type(ET_TASK)) {
+ return false;
+ }
+
+ ConfigInfoDestroyer *destroyer = new ConfigInfoDestroyer(id, info);
+
+ if (eventProcessor.schedule_imm(destroyer, ET_TASK) == nullptr) {
Review Comment:
For what it's worth, I don't understand the comment either.
--
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