stream: hide afterWriteTickInfo property

[Hakerh400]

Pull request #30710 has introduced afterWriteTickInfo property for optimizing synchronous write completions and it is backported to v12.x as a semver-minor release, but it breaks use case of JSON stringifying writable stream object. This PR hides that property behind a symbol.

Fixes #31277

Checklist

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28332/

[BridgeAR]

@Hakerh400 maybe also add a regression test?

[mscdex]

Perhaps we can find a better way to do this?

                                                  confidence improvement accuracy (*)   (**)  (***)
  streams/creation.js kind='duplex' n=50000000           ***    -51.76 %       ±0.96% ±1.27% ±1.66%
  streams/creation.js kind='transform' n=50000000        ***    -48.81 %       ±2.01% ±2.70% ±3.55%
  streams/creation.js kind='writable' n=50000000         ***    -64.69 %       ±2.57% ±3.47% ±4.59%

[BridgeAR]

Using a symbol instead should have less performance impact.

[mscdex]

This also needs a test.

[jasnell]

This is also likely semver-major.

[jasnell]

Moving to an internal symbol and aliasing afterWriteTickInfo is likely the better option here.

[Hakerh400]

Changed to a symbol and added a test.

I don't think this should be a semver-major. The PR that introduced this property was released in a semver-minor release (and could propably be a semver-patch). This PR can be considered as a fix for a broken use case.

[mscdex]

Benchmarks with symbol:

                                                  confidence improvement accuracy (*)   (**)  (***)
  streams/creation.js kind='duplex' n=50000000                   -1.51 %       ±2.24% ±2.99% ±3.93%
  streams/creation.js kind='transform' n=50000000          *      3.69 %       ±3.38% ±4.51% ±5.88%
  streams/creation.js kind='writable' n=50000000                  2.66 %       ±3.54% ±4.71% ±6.13%

[Trott]

@jasnell LGTY?

[ronag]

I'm a little curious about this stringify the stream state use case. That's not really something I believe we support and expecting stringify/parse to work could be a bit dangerous, e.g. if you have callbacks buffered.

I would at least like a test here to avoid similar regressions.

[Hakerh400]

I'm a little curious about this stringify the stream state use case. That's not really something I believe we support and expecting stringify/parse to work could be a bit dangerous, e.g. if you have callbacks buffered.

I agree, it is not documented anywhere that a stream object should not contain circular structures. That is why initially I didn't add any test, but @BridgeAR and @mscdex were explicit about adding a test. The point of this PR is to fix a use case in which a code stopped working after a semver-minor release, which may leave users unprepared. I am more than happy to remove the test if we all agree.

[ronag]

The test is fine. I’m actually asking whether we should add further tests.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28439/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28453/

[Hakerh400]

CI is green.

@jasnell @ronag Any objections left?

[ronag]

LGTM

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/28939/

[Hakerh400]

Given that #31187 has landed without explicit enumerability, I tend to assume that we have decided what the consensus regarding this topic is to omit default values in object property definitions.

Removed default enumerable property. PTAL.

[addaleax]

I fully agree with what @ronag said above – this is a hack to make something work that we don’t actually support. I’m okay with landing this with the understanding that a semver-major revert PR is opened afterwards.