stream: Writable should not invoke _destroy while pending ops

[ronag]

I don't think it is a good idea that a call to Writable.destroy will invoke Writable._destroy while there are still active/pending _write & _writev (or _read for that matter). Since these might be working on resources that might be closed and cause invalid state.

We have already encountered this problem with fs writable streams (#2006). The solution there is a bit of a "hack". I'm sure the same problem can occur in other userland stream implementations as well as other edge cases (including core streams).

I think the "proper" way to resolve this is to invok Writable._destroy after all pending operations are complete (e.g. pendingcb === 0).

I don't have a obvious way right now to do this change as it would require inlining the destroyImpl helper.

I think this applies to Readable as well.

[ronag]

ping @mcollina thoughts?

[mcollina]

destroy() is an immediate action. I’m -1 in having it to wait.

[ronag]

@mcollina: What about pending operations? I would argue that we cannot make destroy an immediate action in the cases where the resources to be destroyed are in active use (especially since we don't have synchronous cancellation). The fs example illustrates this.

[mcollina]

destroy() is an action that will guarantee that there will be no pending operations. The problem with waiting for pending writes is that they might never come: the current model ensures that. Waiting for pending writes might cause memory leaks in implementations that are buggy.

I might be ok in adding an option to write { waitPending: true } so that _destroy() is called after that. However this is going to be tricky as we would have to start erroring while a stream is being destroyed.

[ronag]

destroy() is an action that will guarantee that there will be no pending operations.

But how? We can't guarantee that since we can't cancel already dispatched _write & _writev?

The problem with waiting for pending writes is that they might never come: the current model ensures that. Waiting for pending writes might cause memory leaks in implementations that are buggy.

Maybe some kind of timeout? Which is the responsibility of the _write implementor.

I understand how you would like it to work. But I don't see how we can make it work like that.

#29028 will only ensure that buffered requests are not dispatched. Not that already dispatched requests will be cancelled.

[ronag]

I would like to refer to this issue #2006 which illustrates the problem. The way it was solved is kind of a hack where destroy doesn't actually destroy (https://github.com/nodejs/node/blob/master/lib/internal/fs/streams.js#L392).

[mcollina]

But how? We can't guarantee that since we can't cancel already dispatched _write & _writev?

Let's take a fs.WriteStream as an example. destroy() responsibility is to close the file descriptor as soon as it's possible. It guarantees that there are no leaked file descriptor. destroy() is not a "graceful" action, but rather a disruptive action to be called to tear down state. The normal behavior for writables is to use end() - that implements what you are asking for already.

[ronag]

Hm, that makes sense. Thank you for explaining.