lumberjack
lumberjack copied to clipboard
natefinch
fixed goroutine leak
I consider the Close() method is finalize call, so I make close(chan) call. the relate issue #56
Sorry. I'll check it out tomorrow.
On Sun, Nov 19, 2017, 10:46 PM jack zhao [email protected] wrote:
@jaczhao commented on this pull request.
In lumberjack.go https://github.com/natefinch/lumberjack/pull/57#discussion_r151897771:
@@ -112,6 +112,7 @@ type Logger struct { mu sync.Mutex
millCh chan bool
- wg sync.WaitGroup
@natefinch https://github.com/natefinch could you reply this?
— You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub https://github.com/natefinch/lumberjack/pull/57#discussion_r151897771, or mute the thread https://github.com/notifications/unsubscribe-auth/ADCcyMy1Q27Qsngybx28Rg1TMxHjxTj0ks5s4PX_gaJpZM4QQnbH .
I'll have to refresh myself on this. I think it's probably fine, but I want to make sure. Most people just start one logger and let.it run for the lifetime of the process. But if we can make it clean up better, that's definitely a good thing.
This still needs a couple updates as specified above. I'll do the work myself if @jaczhao isn't interested anymore (not anyone's fault, it's been forever).
@natefinch or @jaczhao any plans on picking this up? I'm running into this now as well. If I don't hear an ACK, will post a new PR that picks up where this left off (will be based on this one and implement the feedback)
Opened https://github.com/natefinch/lumberjack/pull/80 as a PR that implements the requested changes to this PR and also adds a test.
I also opened #100 to address this issue. My PR also fixes the test synchronisation and determinism.
Interesting, such a big and obvious bug, and this is still not merged after 3 years.
It's not a big bug, since you don't normally restart your logging infrastructure multiple times. But you're right that it should get merged.
It's a big bug to me. I have a program with multiple goroutines that each of them writes to a different log file.
@natefinch any new plans about this bug？ there has already 3 PRs , will anyone get merged？
