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

Feature/improved exit logging #1296

Merged
merged 1 commit into from
Jan 14, 2016

Conversation

pvandijk
Copy link
Contributor

As someone completely new to Haraka, i noticed that the console logging was partially broken on my environment (centos/node 5.4), such that error messages from the logger were being discarded as process.exit would simply dump the buffers.
Instead of seeing an error for "Failed to setup listeners: listen EACCES" for port 25, i was left with debug info about connect hooks.

Hopefully this change will make the first time experience for people trying Haraka out a little easier.

Similarly if anything is out of line here please let me know as this is the first PR i've made for Haraka.

This PR changes a few small things:

  • Cut over the existing process.exit calls to use exit() from the exit module, which addresses the buffering issue with the logger
  • Remove the exit parameter from logger.dump_logs - i found it a bit strange that a logging module would be responsible for process termination

@Dexus
Copy link
Member

Dexus commented Jan 13, 2016

I am opposed to this amendment because it is an unnecessary dependency.
This change has absolutely no improvement.

@msimerson
Copy link
Member

Hi @pvandijk, thanks for this PR.

In general, this PR is an improvement. My first reaction was the same as @Dexus, that I didn't like the introduction of another dependency. I'm in the habit of putting all my process.exit calls inside a cleanupAndExit function. However, after looking over the exit module, the single function it introduces, and the cast of characters that also depend on it, I've changed my mind and I'm in favor of adding the dependency and merging this PR as-is.

@smfreegard
Copy link
Collaborator

+1 from me too. It fixes a problem that has bitten me numerous times before.

@msimerson
Copy link
Member

Hey @pvandijk, if you'll rebase and squash this branch I'll go ahead and merge.

…rs are drained, also remove exit parameter from dump_logs (as it's confusing)

Remove exit parameter from dump_logs - this behaviour is very counterintuitive

Add exit module for better terminal logging
@pvandijk pvandijk force-pushed the feature/improved-exit-logging branch from e626ec7 to 34fab73 Compare January 13, 2016 22:34
@pvandijk
Copy link
Contributor Author

Thanks for the feedback - it's now squashed.

msimerson added a commit that referenced this pull request Jan 14, 2016
@msimerson msimerson merged commit a522bfc into haraka:master Jan 14, 2016
@msimerson msimerson mentioned this pull request Mar 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants