Improve dd_library_loader dlclosing#3733
Conversation
|
✨ Fix all issues with BitsAI or with Cursor
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3733 +/- ##
==========================================
+ Coverage 68.78% 68.79% +0.01%
==========================================
Files 166 166
Lines 19015 19015
Branches 1792 1792
==========================================
+ Hits 13079 13081 +2
Misses 5124 5124
+ Partials 812 810 -2
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Without the correct handle, glibc adds to dd_library_loader.c a dependency on ddtrace.so and makes the symbol lookup procedure more complicated than it needs to be. (This still doesn't result in ddtrace.so being unloaded when dlclose() is called on it because ddappsec.so depends on it still. This will be fixed next.)
cataphract
force-pushed
the
glopes/dd-lib-loader-v2
branch
from
27c11a0 to
ceae2a4
Compare
| // Use double-fork to avoid needing a background reaper thread. | ||
| // The intermediate child exits immediately and is reaped synchronously | ||
| // by the parent via waitpid(). The grandchild is reparented to init, | ||
| // which reaps it automatically after it calls execv(). |
There was a problem hiding this comment.
I think I forgot to mention this in the comment :-( - but we have users with "misbehaving" PID 1 scripts in docker containers, which do not reap and accumulate zombies.
That's why we don't double fork. So no, don't just replace that, we have to actually reap this ourselves.
There was a problem hiding this comment.
Because yes, double fork is the obvious solution to this problem, just not an acceptable one here.
| // shutdown callbacks are run in the same order (not reverse) as they | ||
| // were loaded. So the callbacks for ddtrace/appsec/profiling will run | ||
| // AFTER this callback. | ||
| __auto_type ext_config = &ddloader_injected_ext_config[i]; |
There was a problem hiding this comment.
I think we need a lint against __auto_type :-P
In all honesty, I just don't like it in C. It's easy to accidentally alias stuff to the wrong type in C - I think always writing the type is definitely superior.
There was a problem hiding this comment.
well, we can stop using it once we move to C23, because auto has been repurposed there :p
cataphract
force-pushed
the
glopes/dd-lib-loader-v2
branch
2 times, most recently
from
ade5bd1 to
36abf37
Compare
Benchmarks [ tracer ]Benchmark execution time: 2026-03-26 15:42:02 Comparing candidate commit 36abf37 in PR branch Found 1 performance improvements and 3 performance regressions! Performance is the same for 188 metrics, 2 unstable metrics. scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching2-opcache
scenario:TraceSerializationBench/benchSerializeTrace
|
cataphract
force-pushed
the
glopes/dd-lib-loader-v2
branch
from
36abf37 to
71013b9
Compare
github-actions
Bot
added
profiling
3 participants
Description
Improve dd_library_loader dlclosing
Reviewer checklist