bryancall opened a new pull request, #13757:
URL: https://github.com/apache/trafficserver/pull/13757
Every config reload leaked its whole task tree. Under an ASAN build this
fails `traffic_ctl_config_reload`: each reload sub-test passes, but
`traffic_server` exits 1 with a LeakSanitizer report, so the `ts_0` ReturnCode
check fails.
```
Direct leak of 728 byte(s) in 7 object(s) allocated from:
#1 ConfigReloadTask::start_progress_checker()
src/mgmt/config/ConfigReloadTrace.cc:394
...
SUMMARY: AddressSanitizer: 7823 byte(s) leaked in 69 allocation(s).
```
Two independent owners kept the tree alive, and each has to go:
1. **The progress checker never freed itself.**
`ConfigReloadProgress::check_progress()` returned `EVENT_DONE` without `delete
this`, and nothing else references the checker or cancels its event. So each
reload leaked one checker and, through its `_reload` shared_ptr, the main task
it watched. This is the Direct leak: 7 reloads in the test, 7 checkers reached
`EVENT_DONE`, 7 objects leaked.
2. **Parent and child tasks formed a shared_ptr cycle.** A child held its
parent in `_parent` (a `shared_ptr`) while the parent held the child in
`sub_tasks`. After `ReloadCoordinator` dropped a task from `_history`, the
cycle still kept the whole tree alive. `_parent` is now a `weak_ptr`, matching
`ConfigContext`, which already holds its task weakly.
A new unit test in `test_ConfigReloadTask.cc` checks that dropping the main
task frees the tree, and that a child whose parent is gone can still change
state. It fails on master (`weak_main.expired()` is false) and passes with this
change.
### Testing (dev-asan, Fedora 44, GCC 16)
- `test_records`: all 47 test cases pass, with no LeakSanitizer report.
- `traffic_ctl_config_reload`: fails on master, passes with this change, and
`ts_0` exits 0 with no LeakSanitizer output.
- I ran the 43 gold tests that exercise config reload (all except
`remap_acl` and `remap_acl_yaml`) with and without this change. 34 pass. The
other 9 fail on master too, and none of their remaining leaks involve
reload-tracing code. In the ones that leaked reload tasks, the reload part is
gone, e.g. `config_reload_plugin_api` drops from 6892 B to 40 B. The leaks that
remain are separate, pre-existing issues:
- 40 B from `single_plugin_init` (`Plugin.cc:250`)
- an OpenSSL mem-BIO `BUF_MEM` leak in the `tls_client_cert*` and
`tls_sni_yaml_reload` tests
- a `std::string` in `TLSTunnelSupport::set_tunnel_destination`
(`TLSTunnelSupport.cc:79`)
`abuse_shield` fails under parallel load on both trees and passes when run
alone.
This code was backported to 10.2.x in #13354. `ConfigReloadTrace.cc` there
is identical to master, and the header has the same `_parent` member, so this
should be a candidate for 10.2.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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
