nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 227 forks source link

Is there a way to synchronously flush + close a stream? #341

Closed dead-claudia closed 6 years ago

dead-claudia commented 6 years ago

I know this is an unusual request, but I need it to deal with uncaughtException errors in child processes, to send them to the parent to be decoded and handled there safely. Since you aren't supposed to recover from uncaughtException (it's obviously unsafe), I have to re-crash within the listener, which makes any sort of async work impossible.

Here's what I propose:

The alternative here is to drop into native and send it without event loop interference, but I'd strongly prefer to avoid a native dependency if possible, since I also plan to eventually extend this to the browser, and I don't want to have to bundle a whole slew of binaries.

mcollina commented 6 years ago

@isiahmeadows this is very interesting, and I had to deal with this recently. The problem is that we cannot do this for anything but FS and network stream. I recently wrote https://github.com/mcollina/sonic-boom for this specific need.

The problem of flushing synchronously is that some data might get lost if there is another write in process.

I would recommend to re-crash within the listener of uncaughtException, and do a graceful shutdown.

dead-claudia commented 6 years ago

@mcollina

this is very interesting, and I had to deal with this recently. The problem is that we cannot do this for anything but FS and network stream.

I'm aware it can't be done generally - user streams that are asynchronous (i.e. most of them) immediately come to mind. That's why I recommended in the proposed "method" that if it can't flush synchronously.

The problem of flushing synchronously is that some data might get lost if there is another write in process.

I'm aware. For the purposes of my use case, I only need to care about IPC, and I can open a dedicated socket on process creation if necessary. It's doing a last-ditch effort to send a child error to the parent, so the parent can handle not only the child crashing, but why it crashed. By that point, data loss isn't an issue outside this single case of error reporting for me, so I'm not concerned about that risk.

(File system stuff isn't something I need to worry about.)

I would recommend to re-crash within the listener of uncaughtException, and do a graceful shutdown

I'm aware (and that's part of the motivation of this feature request):

Since you aren't supposed to recover from uncaughtException (it's obviously unsafe), I have to re-crash within the listener, which makes any sort of async work impossible.

mcollina commented 6 years ago

I would recommend you to open an issue to nodejs/node for a feature request on flushing the IPC stream for child processes. This repo is more about generic streams, and I do not think we should add that as part of the generic stream interface.