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-pretty breaks flushing of stdout #279

Closed
mojavelinux opened this issue Dec 3, 2021 · 5 comments · Fixed by #280
Closed

pino-pretty breaks flushing of stdout #279

mojavelinux opened this issue Dec 3, 2021 · 5 comments · Fixed by #280
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@mojavelinux
Copy link
Contributor

mojavelinux commented Dec 3, 2021

pino-pretty overrides the end function on the destination to bypass the call to fs.close.

if (destination.fd === 1) {
  // We cannot close the output
  destination.end = function () {
    this.emit('close')
  }
}

In doing so, it breaks flushing. In other words, if there are messages in the buffer, they get dropped when the function emits the close event.

Since sonic-boom 2.4, the call to fs.close is now bypassed, yet the buffer is still flushed. Therefore, I think pino-pretty should remove this logic. Besides, the logic is incomplete anyway as it does not consider stderr.

@mojavelinux
Copy link
Contributor Author

The workaround is to save the end function and reassign it after pino-pretty is initialized:

const end = destination.end
logger = pino(config, createPrettyDestination(destination, colorize))
destination.end = end

@mcollina
Copy link
Member

mcollina commented Dec 4, 2021

I agree, this should be removed from pino-pretty.
Would you like to send a Pull Request to address this issue?

@mcollina mcollina added good first issue Good for newcomers enhancement New feature or request labels Dec 4, 2021
@mojavelinux
Copy link
Contributor Author

Yep! Glad to.

@mojavelinux
Copy link
Contributor Author

Wow, that was a tricky one to write a test for. It took awhile to figure out the minimum code in isolation to trigger the problem. It's pretty clear from the test that SonicBoom is doing the work here, so pino-pretty doesn't need to.

@mojavelinux
Copy link
Contributor Author

Btw, I recognize that calling end on the destination isn't the most typical way of using pino. However, it is important when pino is integrated as a framework to build a logger module for an application. The application knows how to create the stream / destination and when it needs to be flushed / closed. It can't rely on the process exit listener to clean things up as that's not when the application stops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants