Fixed goroutine leak, made tests deterministic #1
Conversation
| @@ -4,9 +4,9 @@ package lumberjack | |||
|
|
|||
| import ( | |||
| "os" | |||
| "sync" | |||
There was a problem hiding this comment.
What's the easiest way to test the linux version?
There was a problem hiding this comment.
add a build tag for only running on linux (if necessary), then run on a linux machine (e.g. Jenkins)
| @@ -408,10 +409,6 @@ func TestMaxAge(t *testing.T) { | |||
| equals(len(b2), n, t) | |||
| existsWithContent(backupFile(dir), b, t) | |||
|
|
|||
| // we need to wait a little bit since the files get deleted on a different | |||
| // goroutine. | |||
| <-time.After(10 * time.Millisecond) | |||
There was a problem hiding this comment.
This was waiting for something that shouldn't happen. Now we'll get a deadlock in the test is there's an extra remove because nobody reads the notifyRemove chan.
| @@ -559,35 +555,38 @@ func TestRotate(t *testing.T) { | |||
| err = l.Rotate() | |||
| isNil(err, t) | |||
|
|
|||
| // we need to wait a little bit since the files get deleted on a different | |||
| // goroutine. | |||
| <-time.After(10 * time.Millisecond) | |||
| filename2 := backupFile(dir) | ||
| existsWithContent(filename2, b, t) | ||
| existsWithContent(filename, []byte{}, t) | ||
| fileCount(dir, 2, t) | ||
| newFakeTime() | ||
|
|
||
| b2 := []byte("bar") |
There was a problem hiding this comment.
Added data so we can see if the files are rotated correctly.
| @@ -165,6 +170,14 @@ func (l *Logger) Write(p []byte) (n int, err error) { | |||
| func (l *Logger) Close() error { | |||
| l.mu.Lock() | |||
| defer l.mu.Unlock() | |||
|
|
|||
| if l.millCh != nil { | |||
| close(l.millCh) | |||
There was a problem hiding this comment.
this is the actual fix for the leak, right? (just making sure I grok it)
|
Is this based on natefinch#100 ? |
Yep. Added that to the description, but you were too fast for me. |
| @@ -4,9 +4,9 @@ package lumberjack | |||
|
|
|||
| import ( | |||
| "os" | |||
| "sync" | |||
There was a problem hiding this comment.
add a build tag for only running on linux (if necessary), then run on a linux machine (e.g. Jenkins)
|
@TimKingtonFC this good to land? |
|
Yes, but I haven't figured out how to run the linux tests. |
This is based on howbazaar's changes with 4a6f656c's comments here: https://github.com/natefinch/lumberjack/pull/100/files
I changed a few things, too.