Multistream should catch Error thrown by thread-stream

[ggrossetie]

The write function provided by thread-stream can throw an Error of the worker has exited or is ending:

https://github.com/pinojs/thread-stream/blob/9bbbcd196234971c4e546590a496199863447e44/index.js#L213-L219

The multistream is calling stream.write(data) without a try/catch meaning that the Error will propagate:

pino/lib/multistream.js

Line 64 in 74893ae

stream.write(data)

As a result, an error will be thrown by the logger function, for instance logger.info().
In my opinion, the logger function should never throw an Error, and even more, when using multi streams where only one stream is "failing".

Alternatively, I think thread-stream should not throw an Error but instead emit an error to notify that the log cannot be written on this stream.

[mcollina]

The problem remains: after the worker exits, you won't be able to log anymore and you should crash your process. Currently multistream is not handling the case where one of the destination streams could "crash": would you like to add support for that?

I'm ok to get a PR to thread-stream that emits that error synchronously (this is important, otherwise you might get a silent fail on exit) instead of throwing it.

[ggrossetie]

Thanks for your reply!

The problem remains: after the worker exits, you won't be able to log anymore and you should crash your process.

I agree but it should crash/restart the worker thread process not the main thread/process right?
Currently, we are using logger.info in an Express middleware and the next middleware will receive an Error. So basically, we fail the HTTP request because the logger wasn't able to send a log to one destination (a TCP server).

I'm ok to get a PR to thread-stream that emits that error synchronously (this is important, otherwise you might get a silent fail on exit) instead of throwing it.

Perfect 👍🏻
Should I close this issue and recreate a new one in thread-stream or do you want to transfer it?

[ggrossetie]

I took a deeper look at the code and it seems that ThreadStream already emits an error event and a close event (with setImmediate) in the destroy function:

https://github.com/pinojs/thread-stream/blob/9bbbcd196234971c4e546590a496199863447e44/index.js#L341-L365

One issue might be that the error event is not immediate and another one is that multistream will still try to write on a closed/destroyed stream. It might be unnecessary to emit the same error over and over again when multistream calls write on a closed stream?
If I replace throw new Error('the worker has exited') by setImmediate(() => { stream.emit('error', new Error('the worker has exited')) }) then we will emit an error every time write is called.

Maybe we should automatically remove a closed/destroyed ThreadStream from the stream list when we receive a error/close event (as you said, the worker has exited and there's nothing we can do)?

[mcollina]

Maybe we should automatically remove a closed/destroyed ThreadStream from the stream list when we receive a error/close event (as you said, the worker has exited and there's nothing we can do)?

Maybe, it would really be up to the user to decide to do so. However It's a PR I'd be willing to review.

[ggrossetie]

I think that 895ee80 is enough (at least for now) since users will be able to listen to the error event on a ThreadStream.

I've added a test case in #1496

[github-actions]

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.