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

Always flush logger at exit #389

Merged
merged 2 commits into from May 6, 2011
Merged

Always flush logger at exit #389

merged 2 commits into from May 6, 2011

Conversation

jasonrudolph
Copy link
Contributor

Remove special-case code in the "runner" command, and replace it with a general solution to ensure that the logger gets flushed at exit. This solution works for "runner", "console", "server", rake tasks, and any other process that loads the Rails environment.

Here's a detailed description of the issue that this commit resolves:

Rake tasks fail to log when running in production mode

In production mode, when you write a single line to the Rails logger via script/runner, it writes the line to production.log. (This is, of course, the expected behavior.)

In production mode, when you write a single line to the Rails logger via rake, it writes nothing to production.log.

It's odd to have this difference in behavior between running via script/runner and running via rake. It seems to violate the principle of least surprise. This gist demonstrates the issue.

@josevalim
Copy link
Contributor

Do you think you can add a test? We have some tests in railties/test/application/* that you could use as example for this one. For instance, you have an example of how to define a rake task here:

https://github.com/rails/rails/blob/master/railties/test/application/rake_test.rb#L37

And an example to assert for the logging here:

https://github.com/rails/rails/blob/master/railties/test/application/rake_test.rb#L73

You just need to force the env to production when running the task.

@jasonrudolph
Copy link
Contributor Author

@josevalim Thanks for pointing me toward those example tests. I've added a test to verify that production rake tasks do indeed flush the logger.

@josevalim
Copy link
Contributor

Github says the pull request cannot be automatically merged. Could you please rebase? I will merge it straight away then!

Prior to this change, running code via script/runner would demonstrate
different logging behavior than running the same code via a rake task.
In production mode the script/runner approach would always flush the
logger, but the rake-based approach would not automatically flush the
logger. This discrepancy violates the principle of least surprise, and
it could lead to the loss of important production logging data.

This change removes special-case code in the "runner" command, and
replaces it with a general solution to ensure that the logger gets
flushed at exit. This solution works for "runner", "console", "server",
rake tasks, and any other process that loads the Rails environment.
@jasonrudolph
Copy link
Contributor Author

@josevalim Rebased and pushed. Thanks for the quick response!

josevalim added a commit that referenced this pull request May 6, 2011
@josevalim josevalim merged commit 6acb858 into rails:master May 6, 2011
@pauldowman
Copy link

Excellent, thanks! I was just about to do this myself. :-)

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.

None yet

3 participants