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

pino.final without a handler and stream.flushSync() #1126

Closed
javiertury opened this issue Sep 12, 2021 · 2 comments · Fixed by #1127
Closed

pino.final without a handler and stream.flushSync() #1126

javiertury opened this issue Sep 12, 2021 · 2 comments · Fixed by #1127

Comments

@javiertury
Copy link
Contributor

javiertury commented Sep 12, 2021

I discovered that pino.final can be used without a handler, which I find very handy. Example:

const handler = (err) => {
  const finalLogger = pino.final(logger);
  
  if (err) finalLogger(err);
  process.exit(err ? 1 : 0);
};

But reading the code I found something that might be a bug and perhaps someone here can confirm it.

pino/lib/tools.js

Lines 463 to 478 in c57c258

if (!hasHandler) {
return finalLogger
}
return (err = null, ...args) => {
try {
stream.flushSync()
} catch (e) {
// it's too late to wait for the stream to be ready
// because this is a final tick scenario.
// in practice there shouldn't be a situation where it isn't
// however, swallow the error just in case (and for easier testing)
}
return handler(err, finalLogger, ...args)
}
}

stream.flushSync() is only called before the handler is executed or after finalLogger is used. In the example above there is no handler and finalLogger is only called if there is an error, so when the process exits normally there is no call to stream.flushSync().

Perhaps the implemention when there is no handler should be this

  if (!hasHandler) {
    try {
      stream.flushSync()
    } catch (e) {
      // it's too late to wait for the stream to be ready
      // because this is a final tick scenario.
      // in practice there shouldn't be a situation where it isn't
      // however, swallow the error just in case (and for easier testing)
    }
    return finalLogger
  }
@javiertury javiertury changed the title pino.final and stream.flushSync() pino.final without a handler and stream.flushSync() Sep 12, 2021
@javiertury javiertury changed the title pino.final without a handler and stream.flushSync() pino.final without a handler and stream.flushSync() Sep 12, 2021
@mcollina
Copy link
Member

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

mcollina pushed a commit that referenced this issue Sep 13, 2021
…1127)

* fix: make pino.final sync flushes when no handler provided (#1126)

* refactor(final): remove unused error in catch

Co-authored-by: James Sumners <james@sumners.email>

Co-authored-by: James Sumners <james@sumners.email>
@github-actions
Copy link

github-actions bot commented Feb 3, 2022

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants