Fix logging segfault by leaking logger#15884
Conversation
|
I am exploring an alternative patch wherein I change all the logger references to |
lisanna-dettwyler
force-pushed
the
logging-segfault-leak
branch
2 times, most recently
from
7e8e6d3 to
9dc5c67
Compare
|
I couldn't get the shared_ptr version to work without crashing because we can't release it on fork, so switching back to this version. |
| std::unique_ptr<Logger> | ||
| makeTeeLogger(std::unique_ptr<Logger> mainLogger, std::vector<std::unique_ptr<Logger>> && extraLoggers); | ||
| Logger * makeTeeLogger(Logger * mainLogger, std::vector<Logger *> && extraLoggers); | ||
|
|
||
| std::unique_ptr<Logger> makeJSONLogger(Descriptor fd, bool includeNixPrefix = true); | ||
| Logger * makeJSONLogger(Descriptor fd, bool includeNixPrefix = true); | ||
|
|
||
| std::unique_ptr<Logger> makeJSONLogger(const std::filesystem::path & path, bool includeNixPrefix = true); | ||
| Logger * makeJSONLogger(const std::filesystem::path & path, bool includeNixPrefix = true); |
There was a problem hiding this comment.
This code doesn't need to change though, right?
The only things that needs to change is that the top-level logger is just a raw pointer that gets leaked?
All this plumbing/composition code can stay with unique_ptr, but it's the places where we assign to the global that change?
Is there something I'm missing? That seems like a much smaller change.
There was a problem hiding this comment.
I think there would be no point to having these be unique_ptrs because we'd have to call release() on their outputs anyways, kinda defeating the point of using a unique_ptr in the first place, right?
There was a problem hiding this comment.
Well, for one that's a smaller diff and the it's more about how the logger object itself is global (but the classes themselves don't know about it, and don't have to be used as globals).
Maybe at some point we could improve upon this situation and then churn would hurt more.
There was a problem hiding this comment.
Latest commit is the most minimal change I can do ^-^
There is a race between activity and logger teardown, so we switch from a unique_ptr to a raw pointer and leak the logger so it persists across all activity teardowns. Signed-off-by: Lisanna Dettwyler <lisanna.dettwyler@gmail.com>
lisanna-dettwyler
force-pushed
the
logging-segfault-leak
branch
from
9dc5c67 to
be84169
Compare
3 participants
There is a race between activity and logger teardown, so we switch from a unique_ptr back to a raw pointer and leak the logger so it persists across all activity teardowns.
Motivation
The functional tests are currently randomly segfaulting when trying to use a logger reference in the activity destructor.
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.