fixed goroutine leak #57
Conversation
jaczhao
force-pushed
the
goroutine_leak
branch
from
c9a4468
to
c216329
Compare
jaczhao
force-pushed
the
goroutine_leak
branch
from
87cc1e6
to
e72f7fe
Compare
|
|
||
| if l.millCh != nil { | ||
| close(l.millCh) | ||
| l.wg.Wait() |
There was a problem hiding this comment.
after waiting, you need to set millCh to nil, so if someone calls close again, it won't panic, trying to close the same channel.
| @@ -112,6 +112,7 @@ type Logger struct { | |||
| mu sync.Mutex | |||
|
|
|||
| millCh chan bool | |||
| wg sync.WaitGroup | |||
There was a problem hiding this comment.
why? it would not be passed by method, just one property of this struct.
There was a problem hiding this comment.
@natefinch could you reply this?
There was a problem hiding this comment.
@jaczhao
https://golang.org/pkg/sync/#WaitGroup
A WaitGroup must not be copied after first use.
There was a problem hiding this comment.
so where to init the : wg sync.WaitGroup
temporary I put it in file : lumberjack.go
// mill performs post-rotation compression and removal of stale log files,
// starting the mill goroutine if necessary.
func (l *Logger) mill() {
if l.wg == nil {
l.wg = new(sync.WaitGroup)
}
l.startMill.Do(func() {
l.wg.Add(1)
l.millCh = make(chan bool, 1)
go l.millRun()
})
select {
case l.millCh <- true:
default:
}
}
| @@ -623,7 +623,7 @@ func TestCompressOnRotate(t *testing.T) { | |||
|
|
|||
| // we need to wait a little bit since the files get compressed on a different | |||
| // goroutine. | |||
| <-time.After(10 * time.Millisecond) | |||
| <-time.After(20 * time.Millisecond) | |||
There was a problem hiding this comment.
Did you need to up these to make the tests pass? Just curious
There was a problem hiding this comment.
yes, I would got random failed if only 5 seconds
|
Sorry. I'll check it out tomorrow.
…On Sun, Nov 19, 2017, 10:46 PM jack zhao ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lumberjack.go
<#57 (comment)>:
> @@ -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
<#57 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADCcyMy1Q27Qsngybx28Rg1TMxHjxTj0ks5s4PX_gaJpZM4QQnbH>
.
|
|
@natefinch any news? |
|
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 #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. |
|
so cool, the bug has not been fixed yet. |
Unfortunately not it seems. |
|
@natefinch any new plans about this bug？ there has already 3 PRs , will anyone get merged？ |
|
We should create a new repo and dispose this one. |
**Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> This enables `goleak` to check for any leaking goroutines in this package. There is an existing goleak, but it's in a third party dependency which we cannot resolve on our end. There's been a [PR open for this issue](natefinch/lumberjack#57) for 6 years and it hasn't been merged. **Note:** The `natefinch/lumberjack` package is now unmaintained, so this will likely never be fixed. We should consider moving to a new package if possible to avoid the leak. I'll defer to code owners for feedback on possible options. **Link to tracking Issue:** <Issue number if applicable> #30438 **Testing:** <Describe what testing was performed and which tests were added.> Existing tests and added goleak check are passing.
) **Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> This enables `goleak` to check for any leaking goroutines in this package. There is an existing goleak, but it's in a third party dependency which we cannot resolve on our end. There's been a [PR open for this issue](natefinch/lumberjack#57) for 6 years and it hasn't been merged. **Note:** The `natefinch/lumberjack` package is now unmaintained, so this will likely never be fixed. We should consider moving to a new package if possible to avoid the leak. I'll defer to code owners for feedback on possible options. **Link to tracking Issue:** <Issue number if applicable> open-telemetry#30438 **Testing:** <Describe what testing was performed and which tests were added.> Existing tests and added goleak check are passing.
I consider the Close() method is finalize call, so I make close(chan) call. the relate issue #56