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

Maybe a bug of reopen_files when handle signal USR1 ? #1739

Closed
ysymi opened this Issue Apr 2, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@ysymi
Contributor

ysymi commented Apr 2, 2018

I am sorry I don't know how to discuss on mailing list.

phenomenon:

when gunicorn received signal USR1, my flask app's logger will write log to another file, which is unexpected.

reason:

my logger in flask app use handler implemented by myself, and my handler does not open file of handler.baseFilename. when gunicorn worker handle USR1, it will search all logger in that gunicorn worker process and reopen the handler's stream by open(handler.baseFilename).

related code:

# loggers method core logic
root = logging.root
existing = root.manager.loggerDict.keys()
return [logging.getLogger(name) for name in existing]

# reopen_files method core logic
for log in loggers():
    for handler in log.handlers:
        if isinstance(handler, logging.FileHandler):
            if handler.stream:
                handler.stream.close()
                handler.stream = open(handler.baseFilename,  handler.mode)

my idea:

To be honest, I do not understand why gunicorn need reopen all logger in a python process , shouldn’t it pick out only gunicorn's logger ?however, if it's really required, the reopen_files logic must be safe or do not have effect to other logger at least.
Many people use their own logger class or handler, they may have different implementation, but, they must overwrite same name method in python logging module. If gunicorn want to reopen all logger, it can use FileHandler's method close and _open(see here, line 898 )to support different custom logger or handler.

I am happy to submit a pr to optimize here :)

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Apr 2, 2018

To be honest, I do not understand why gunicorn need reopen all logger in a python process , shouldn’t it pick out only gunicorn's logger?

I think I agree with that. That would be a small breaking change, so I think I like your other idea. Let's use close and _open. Using a _ method doesn't feel any worse than using two undocumented properties, and at least it can be overridden.

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Apr 2, 2018

I'd review a PR for this.

@ysymi

This comment has been minimized.

Contributor

ysymi commented Apr 3, 2018

@tilgovi thanks for your comment. I will prepare a pr as quickly.
and I want to know, in your opinion, what is purpose of gunicorn need to process all logger (except root logger, of course) ?

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Apr 4, 2018

what is purpose of gunicorn need to process all logger

I cannot remember any discussion of this. It probably just seemed sensible at the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment