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

Support synchronous logging for proper exit handling #13

Closed
omidkrad opened this issue Oct 23, 2019 · 5 comments
Closed

Support synchronous logging for proper exit handling #13

omidkrad opened this issue Oct 23, 2019 · 5 comments

Comments

@omidkrad
Copy link
Contributor

omidkrad commented Oct 23, 2019

Exit logging does not work on pino-seq which is crucial for production code. This causes important error logs to get lost on unhandled exceptions. When using exit handlers I get the following error:

Error: final requires a stream that has a flushSync method, such as pino.destination and pino.extreme

These issues pinojs/pino-pretty#37 and pinojs/pino#542 seem to be related. We need seq-logging to support synchronous requests for this to work so we can call it from flushSync implementation in pino-seq and probably other implementors.

omidkrad referenced this issue in simihartstein/pino-seq Oct 23, 2019
@nblumhardt
Copy link
Member

Thank you for raising this 👍

Do you have any idea which is the preferred Node API for implementing this?

@omidkrad
Copy link
Contributor Author

I don't think it needs a Node API, but needs to synchronously flush the logs or wait until all async writes are completed before returning.

My workaround right now is to wait a few seconds and hope that all logs are flushed before exiting.

setTimeout(() => {
   process.exit(1);
}, 5000);

@nblumhardt
Copy link
Member

Thanks for the follow-up. I think the origin of the sync requirement is because queued work (such as timeouts) won't be completed in certain process exit scenarios. I believe this is where the need for a synchronous request comes into the picture.

@rj-xy
Copy link
Contributor

rj-xy commented Jul 13, 2021

Perhaps pino-seq should implement something like this: index.ts#L59

When winston.close() is called, the seq-logger (winston-seq) will flush and close, all the while emitting events respectively.
The subscribed events are then called (integration.test.ts#L24) and the async Test can then end.

winstonjs only provides a synchronous close method, so I've had to emit events so that I know the logger has been flushed and closed, to finalise my integration test.

@nblumhardt
Copy link
Member

Hi all! Awaiting the logger's flush() method should now work, and no synchronous APIs should be needed:

await logger.flush();

If you still run into any problems or incompatibilities with this please let me know; we'll close this because the codebase has changed drastically since the original issue report was received, and it's not clear that the same constraints/deficiencies still exist today. Thanks!

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

No branches or pull requests

3 participants