Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid persistent mill goroutine #90

Closed

Conversation

jdhenke
Copy link

@jdhenke jdhenke commented Aug 6, 2019

Previously, each lumberjack logger starts its own mill goroutine the first time
that the logger writes to a file, and the goroutine is never stopped. The
project has an open pull request,
#80, to stop the goroutine when the
logger is closed. However, even with that design, each open logger will always
have a mill goroutine that is active (even if it is not performing work). It
also means that a logger will never be garbage-collected (and hence its file
will never be closed) unless it is explicitly closed. Furthemore, even if a
logger is explicitly closed, if Write is called on any references to that
logger, that will restart the persistent mill goroutine and reintroduce the
leaked file handle.

This change modifies the mill goroutine logic. It still maintains the overall
design of synchronously rotating and asynchronously deleting and compressing
(milling) the old, rotated log files, and that if a request to mill comes in
while one call is already happening, another mill will be executed after the
current one to ensure any newly rotated files are not missed. There is at most
one active mill goroutine which is in the process of performing a single pass
of deletions and compressions of rotated log files, and at most one queued mill
goroutine which is just waiting on a mutex. Now, each mill goroutine exits once
a single pass is done and future calls to mill will start a new goroutine (if
one is not already queued.)

This modification allows for lumberjack loggers to be garbage-collected, which
allows it to close file handles gracefully without an explicit call to "close".

The downside to this new approach is in the case where rotations are constantly
happening, rather than having a single goroutine simply process each mill pass,
a new goroutine must be started for each pass.


We have an internal fork of lumberjack that contains this change that we are
using in production.

An alternative approach was sketched after the fact that is not in use
internally but would have at most one mill goroutine per logger at any time,
rather than one active and one queued as in this approach. This alternative
design is shown here:

jdhenke#2

Previously, each lumberjack logger starts its own mill goroutine the first time
that the logger writes to a file, and the goroutine is never stopped. The
project has an open pull request,
natefinch#80, to stop the goroutine when the
logger is closed. However, even with that design, each open logger will always
have a mill goroutine that is active (even if it is not performing work). It
also means that a logger will never be garbage-collected (and hence its file
will never be closed) unless it is explicitly closed. Furthemore, even if a
logger is explicitly closed, if Write is called on any references to that
logger, that will restart the persistent mill goroutine and reintroduce the
leaked file handle.

This change modifies the mill goroutine logic. It still maintains the overall
design of synchronously rotating and asynchronously deleting and compressing
(milling) the old, rotated log files, and that if a request to mill comes in
while one call is already happening, another mill will be executed after the
current one to ensure any newly rotated files are not missed. There is at most
one active mill goroutine which is in the process of performing a single pass
of deletions and compressions of rotated log files, and at most one queued mill
goroutine which is just waiting on a mutex. Now, each mill goroutine exits once
a single pass is done and future calls to mill will start a new goroutine (if
one is not already queued.)

This modification allows for lumberjack loggers to be garbage-collected, which
allows it to close file handles gracefully without an explicit call to "close".

The downside to this new approach is in the case where rotations are constantly
happening, rather than having a single goroutine simply process each mill pass,
a new goroutine must be started for each pass.

---

We have an internal fork of lumberjack that contains this change that we are
using in production.

An alternative approach was sketched after the fact that is not in use
internally but would have at most one mill goroutine per logger at any time,
rather than one active and one queued as in this approach. This alternative
design is shown here:

#2
@jdhenke jdhenke force-pushed the avoid-persistent-mill-goroutine branch from 03be6e2 to f05c9ff Compare August 6, 2019 23:49
@jdhenke
Copy link
Author

jdhenke commented Aug 6, 2019

FYI, looks like TestCompressOnRotate flakes.

These commits have the same diff:

@natefinch
Copy link
Owner

Thanks, I'll take a look

@howbazaar
Copy link

One of the problems with this is that there is the possibility of having multiple calls to millRunOnce at one time. I worked on my own fix for this before I realised that there were previous PRs, #100.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants