stream: call _final synchronously

[ronag]

• We don't actually have to call _final in a nextTick we just need to emit the event asynchronously. Try to call _final as soon as possible. With this change it's possible to remove some prefinish hacks where prefinish is used to get a synchronous pre _final.

This is blocking some other pending fixes.

Blocked by #29656

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ronag]

Those failures are very interesting. WIP

[ronag]

@mcollina: I think you might be interested in this change.

@jasnell@addaleax The http2 failures are very strange to me. The way http2 uses streams a bit of a challenge. My gut feeling is that there is a bug hiding somewhere and this PR activates it. Would either if you be interested in collaborating with me to figure out why this PR would cause the failing tests in http2?

[ronag]

e.g. in test-http2-client-upload-reject

server.on('stream',common.mustCall((stream)=>{// Wait for some data to come through.setImmediate(()=>{stream.on('close',common.mustCall(()=>{assert.strictEqual(stream.rstCode,0);}));stream.respond({':status': 400});stream.end();});}));

stream won't emit 'close' until the reading side has also emitted 'end' (due to allowHalfOpen = true in the HTTP2Stream constructor). However, since this test never starts to read it should actually never emit 'end' and 'close' so I'm unsure how this test could have passed previously?

The rest of the failures seem related to this.

[addaleax]

Would this maybe be a good reason to keep _final behind a nextTick, at least in a semver-patch PR? Or maybe, why does this need to be changed here but not in the other places where we use a similar pattern (e.g. net, http2)?

[ronag]

That's one reason to keep _final behind a nextTick but there are other reasons which make it not a good idea. I don't think this becomes a relevant problem once #29656 is merged.

[ronag]

but not in the other places where we use a similar pattern (e.g. net, http2)?

It should probably be changed in those places as well.

[ronag]

@Trott a WIP label please

[ronag]

This has some overlap with #30733. This is more about calling _final synchronously as to avoid 'prefinish' hacks.

[mcollina]

I’m positive to this change, however I would wait for #29656 to land. I’d just be cautious and flag it semver-major. All things that interact with the timing order are hard to debug.

[ronag]

Using this transform stream could be potentially significantly simplified, see https://github.com/nxtedition/node/blob/stream-transform-simplify/lib/_stream_transform.js (WIP). As well as other "sync" streams.

[ronag]

@mcollina@Trott this is no longer WIP

[ronag]

@addaleax Maybe you have some insight here?

[addaleax]

What happens when it isn’t? 😅

[ronag]

Oh! I forgot about that small detail:

Path: parallel/test-http2-client-socket-destroy
Mismatched noop functioncalls. Expected exactly 1, actual 0.
at Object.mustCall (/Users/ronagy/GitHub/nxtedition/node/test/common/index.js:324:10)
at Http2Server.<anonymous> (/Users/ronagy/GitHub/nxtedition/node/test/parallel/test-http2-client-socket-destroy.js:19:29)
at Http2Server.<anonymous> (/Users/ronagy/GitHub/nxtedition/node/test/common/index.js:358:15)
at Http2Server.emit (events.js:304:20)
at ServerHttp2Session.sessionOnStream (internal/http2/core.js:2739:19)
at ServerHttp2Session.emit (events.js:304:20)
at emit (internal/http2/core.js:285:8)
at processTicksAndRejections (internal/process/task_queues.js:87:22)
Command: out/Release/node --expose-internals /Users/ronagy/GitHub/nxtedition/node/test/parallel/test-http2-client-socket-destroy.js

[ronag]

Seems like some form of timing issue.

[ronag]

This PR makes the order here more sensible

[ronag]

This is semver-major due to the change in timing

[ronag]

@Trott: node/streams ping?

[mcollina]

I’m not convinced by this change given the impact in our codebase.

[ronag]

My main motivation is that it would be significantly easier to implement streams which need to support both sync and async behavior, e.g. Transform. See for example, https://github.com/nxtedition/node/blob/stream-fix-end-finish/lib/_stream_transform.js vs https://github.com/nodejs/node/blob/master/lib/_stream_transform.js.

This is currently not possible. It's made worse by the fact that you can't properly emulate this with prefinish hacks since prefinish will be emitted sync if _final === null but async otherwise.

Though this doesn't actually "fix" anything other than make things easier to maintain. So whether it's worth the risk or not is debatable...

[ronag]

I think there are more pressing PR's to land so if this is controversial I'm fine with closing this for now and re-open at a more appropriate time.

[ronag]

The prefinish sometimes sync and sometimes async thing is something I just noticed today. Not sure if there are and what the implications of that might be, i.e. a stream might behave differently if it uses prefinish and sometimes has and sometimes doesn't have a _final.

Example:

letticked=false;constt1=newTransform({transform(chunk,encoding,cb){cb();}});t1.on('finish',()=>{console.log('t1',ticked);});t1.end();constt2=newTransform({transform(chunk,encoding,cb){cb();},final(callback){callback();}});t2.on('finish',()=>{console.log('t2',ticked);});t2.end();ticked=true;// Prints// t1 false// t2 true

Though providing a _final to a Transform stream doesn't make much sense (should use _flush). But it does illustrate the potential of difference in behavior.

[ronag]

Pushed a stream: simplify Transform commit to better show why this PR is useful (still some tests failing), i.e. making it possible to implement Transform strictly in terms of Duplex without extra state and logic. I would prefer to continue working on the Transform part as a separate future PR.

[ronag]

Yea, I still think this is a good idea long term but now that I started digging into it again it's significantly more complicated than I thought. We have higher prio stuff to put energy into.

I'll re-open this in the future.