finished(stream, cb) invokes callback too early

[ronag]

The semantics of finished is to invoke the callback on 'end' and 'finish'. The problem with this is that this does not mean that the stream is actually "finished", i.e. it might still emit 'error' and has probably not yet freed all resources since 'close' is not yet emitted.

The reason we invoke it early in this way is because not all stream actually emit 'close'. I'm a little unsure how to resolve this and whether this is worth resolving.

One way could be to use setImmediate or setTimeout before invoking the callback in 'end'/'finish' in order to give a chance for 'error' or 'close' to be emitted. However, that is very much imperfect.

We might want to at least clarify this in the docs.

Thoughts? @nodejs/streams

[mcollina]

I think the current behavior is fine.

What would be the actual difference in waiting for close instead? (minus backward compatibility, which would postpone any changes for a long time :/).

[ronag]

What would be the actual difference in waiting for close instead?

I guess 2 things, an error from resource cleanup might be swallowed also making assumption in regards to whether _destroy has actually finished might fail:

consider:

// Creates internally a file called "foo.tmp"
const s = new MyStream();
s._destroy((err ,cb) => {
  fs.unlink("foo.tmp", cb);
});
finished(s, (err) => {
  // Code that assumes that _destroy has completed, e.g. "foo.tmp" has been removed.
});

I'll give that I'm grabbing a bit at straws but I still think it's worth at least explicitly mentioning in the docs.

[vweevers]

I've been bitten by this too, but as y'all mentioned, there's only one proper way to solve it, namely having all streams emit close.

+1 for documenting it until then.

[ronag]

There is also the option to assume 'close' will be emitted for "modern" streams and only use this behavior for "legacy" streams.

e.g.

const s = s._writableState || s._readableState;
const isLegacy = !s || s.closed === undefined;

However, that would assume that "modern" streams are properly implemented in terms of autoDestroy and emitClose which I've noticed is not always the case.

[vweevers]

I think ultimately, autoDestroy should be the default behavior, without being able to opt-out.

[ronag]

I think ultimately, autoDestroy should be the default behavior, without being able to opt-out.

It is default in master. Not being able to opt-out is going to be difficult. We have some code in core that depends on it which I find unlikely to be refactored to such a degree (though other parts are work in progress).

[ronag]

We could also go even more conservative:

const s = s._writableState || s._readableState;
const isLegacy = !s || s.closed === undefined || !s.autoDestroy || !s.emitClose;

[vweevers]

If it (finishing on close) only works for some streams, it might not be worth it, because you can't rely on it without checking the implementation details of every stream.

[vweevers]

What if streams had a way to say "yes, I always emit close"? Like s.willEmitClose (bikeshed name).

[ronag]

What if streams had a way to say "yes, I always emit close"? Like s.willEmitClose (bikeshed name).

streams should always emit 'close'. Such a property would only say "yes, I'm a broken stream".

[vweevers]

Exactly, haha. There are too many streams out there that by today's standards are broken. The only sensible value for s.willEmitClose is true, but it'd be a new property that defaults to false. Module authors must explicitly set it to true. The point is to differentiate a stream from old streams, as well as userland streams that do depend on latest readable-stream but still do things the old way.

(I would strongly prefer a solution that's baked into streams and doesn't require action from module authors, but it seems that's not an option due to aforementioned node internals, so here we are).

[ronag]

I've also noticed that https://github.com/nodejs/node/blob/master/lib/internal/streams/pipeline.js#L44 depends on this behavior where finished/eos is invoked as early as possible.

[vweevers]

Yeah, stream.pipeline() is where I was bitten. I had a stream that in essence owned a lock and had to be unlocked in its _destroy() method. On top of that, when the pipeline finished, a new one started that sometimes needed the same lock, along the lines of:

function loop() {
  stream.pipeline(getStreamWithLock(), ..., function () {
     loop()
  })
}

[mcollina]

I'm definitely +1 if we can soft-detect this on a stream-by-stream basis

[ronag]

What are the preferred semantics here? I think pipeline should preferrably only call the callback once every stream has emitted 'close' with fallback behavior for legacy streams?

@mcollina: Even if we exclude "legacy" streams (we can detect pre v14), we would potentially break "modern" streams that explicitly set autoDestroy: false, emitClose: false or monkey patch destroy in a way that causes 'close' never to be emitted. They are essentially "broken", but this kind of change could make them break harder (which might or might not be a good thing depending on perspective). Do we think that is feasible? I wouldn't mind creating a PR and trying CITGM if we think this would be acceptable if CITGM passes.

[vweevers]

They are essentially broken, but this kind of change could make them break harder

Broken according to modern stream semantics, but still usable. It would have been different if pipeline() wasn't already in the wild. Folks are using it with every kind of stream. Including streams that have their own destroy, back from when node didn't have that yet, so it might not be fair to classify that as monkey-patching. But those streams are probably compatible, seeing as they inspired the modern destroy.

If we favor backwards compatibility and can't reliably detect whether a particular stream will emit close, or invent a way for the stream to signal that it will emit close, we must assume it doesn't. On the other hand, I want streams to move forward (until we can say "it just works™") so I'm torn.

[vweevers]

I don't think CITGM would give us enough coverage, because every module must have its own tests for the close event.

[vweevers]

This reminds me of stream-test (see vweevers/stream-test#2) which never got anywhere because I didn't have the time.

[mcollina]

From my point of view, I think we should check if autoDestroy and emitClose options are set, and in that case wait for 'close'. Otherwise keep the current backward compatible behavior.