Attempt to remove *very small* leak for Logger #5579
Conversation
|
I have a vague recollection of trying that first when I wrote this, and then encountering some weird segfaults so falling back to the leaky-but-working plain pointers. But I’d be very interested if you can find the reason for the failures :) |
| { | ||
| return new ProgressBar( | ||
| return std::unique_ptr<Logger>(new ProgressBar( |
There was a problem hiding this comment.
std::make_unique please. The current proposal has a race condition where throwing as part of the ProgressBar constructor would still leak memory.
src/libstore/daemon.cc
Outdated
| @@ -944,16 +944,21 @@ void processConnection( | |||
| throw Error("the Nix client version is too old"); | |||
|
|
|||
| auto tunnelLogger = new TunnelLogger(to, clientVersion); | |||
| auto prevLogger = nix::logger; | |||
| auto prevLogger = nix::logger.get(); | |||
There was a problem hiding this comment.
You could std::move it and construct the TunnelLogger with std::make_unique instead.
This wouldn't require dealing with any raw pointers at all. Also we don't have to recreate a new smart pointer for the tunneLogger further down. Right now this is using a dangling pointer (as the unique_ptr was overriden which frees the previous memory it pointed at).
There was a problem hiding this comment.
Here was the problem with this code:
If I std::move it to prevLogger, then nix::logger is invalid. It is only set to the tunnelLogger if its !recursive; so it has to remain correct otherwise.
I also can't simply assign nix::logger to the tunnelLogger as that will destroy the previousLogger which is called in the Finally block later.
It's a very tricky piece of code to get right with unique_ptr -- perhaps nix::logger should be shared_ptr instead or I can get make the prevLogger a const
src/libstore/daemon.cc
Outdated
|
|
||
| unsigned int opCount = 0; | ||
|
|
||
| Finally finally([&]() { | ||
| _isInterrupted = false; | ||
| prevLogger->log(lvlDebug, fmt("%d operations", opCount)); | ||
| delete(prevLogger); |
There was a problem hiding this comment.
Just don't start doing that. Use smart pointers all the way if we do it. Keep them "smart" that way it'll cleanup for us.
There was a problem hiding this comment.
Please see the comment above -- happy to keep discussing it. I couldn't make it work easily.
|
@regnat I think I got some of the segmentation fault fixed -- unfortunately one remains in the recursive nix test.... |
|
@regnat GDB was helpful in finding at least some of the segfaults In this case it was that JSONLogger needs to take ownership of the loggers being passed in. |
baef428 to
d506b9d
Compare
| if (!state->active) { | ||
| // wait for the thread to complete to avoid | ||
| // 'terminate called without an active exception' | ||
| if (updateThread.joinable()) updateThread.join(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Would love feedback here.
This was kind of tricky because it looked like in some cases the ProgressBar class was destroyed while the thread was still active which raised an error.
I then tried to add join but I believe that there is a timing issue here that the thread was not even started.
Still feels like a data race.
b9fc301 to
b8256b2
Compare
|
Tests are timing out at 60m for some reason but they look healthy. |
|
@regnat looks like it's timing out after 60m, which is surprising. The logs show it's still trying to build. Not sure why this PR takes so long to build. |
The last steps of the build are actually using the freshly built Nix. So maybe there’s something in the changes that causes Nix to hang (I guess that would be https://github.com/NixOS/nix/pull/5579/files#diff-3dbafb27c00d5891f1be772269e9979627f31e3ec1545660f7278523dda23606R112-R117 , but I’m not sure why it would hang ) |
|
TBH, if we are stuck on weird bugs, I would just use |
|
@Ericson2314 has a point indeed :) Esp. in this case where using a shared_pointer won’t have even a remotely measurable impact |
|
I marked this as stale due to inactivity. → More info |
|
On a separate note, it would be good to get whatever static analyses you were running in CI! |
|
Making as draft because IIRC this doesn't quite work. |
I'm running through running various sanitizers on the Nix project.
This was one leak that looked like it could be fixed intentionally using unique_ptr; granted the amount of memory here is pretty small.
Here is the leak that is fixed:
Looks like there are in fact a few more given how that the loggers are just re-assigned over each other.
According to some discussion on Matrix, the Nix tool intentionally leaks memory through the AST/parser.
Perhaps it's worth investigating somehow disabling these leaks from the sanitizer so that at least they can be excluded from the result since they are quite noisy.