search

natefinch / lumberjack

lumberjack is a log rolling package for Go
MIT License
4.79k stars 591 forks source link

Ensure that the millRun goroutine terminates when Close called. #100

Open howbazaar opened 4 years ago

howbazaar commented 4 years ago

Currently the millRun goroutines leaks. This is very noticable if a Logger is constructed periodically, used and then closed.

This change ensures that the millCh channel is closed if it exists.

Existing log rotation tests cover the duplicate Close calls.

howbazaar commented 4 years ago

I notice that the travis-ci has failures. Interestingly I noticed that could happen when I was reading the tests as there is no synchronisation between the goroutine running the millRunOnce and the tests, so the test triggers the mill, but doesn't wait for the milling to complete before it checks the files.

Do you have any preference on how you'd like to see the synchronisation added?

natefinch commented 4 years ago

No preference, do whatever you feel is best and is most straight forward. I'm not too picky. I really need to make a v3 of this thing and clean some of this stuff up.

howbazaar commented 4 years ago

Wow, adding synchronisation and determinism has proved to be way more complicated than I thought.

howbazaar commented 4 years ago

FYI, here is the comment I wrote while evaluating the race in TestCleanupExistingBackups:

       // There is a race here between the first mill request which happens
       // when openExistingOrNew starts, and the mill that happens as a part
       // of rotate, that gets called due the the size written into the log file.
       // If the first mill managed to process the old log file before the mill
       // request due to rotate, then we get a notification and may get three
       // files back, as the rotate call creates an additional log file. The
       // second mill call (due to rotate) will remove that additional file.
       // If there is a second call, we need to call waitForNotify or the Close
       // method blocks.
howbazaar commented 4 years ago

OK, after disussion with @4a6f656c, realised that we really do need the rotate on resume functionality.

To make the test deterministic, I changed it to not write enough to trigger a rotate. This way we always just have one notification.

haykbaluyan commented 4 years ago

@howbazaar sorry bothering, but when can we expect this to merge?

hloeung commented 4 years ago

@howbazaar sorry bothering, but when can we expect this to merge?

One more for @natefinch

borud commented 2 years ago

Any progress on this?

DellCurry commented 1 year ago

Would this PR (or #57 / #80 which solve the same bug) merged? I think it is really a big bug to many go developers since zap officially recommend this repo to rotate log file.

SimonRichardson commented 3 months ago

This is now superseded by https://github.com/natefinch/lumberjack/pull/211