stream: construct by ronag · Pull Request #29656 · nodejs/node · GitHub

Provide a standardized way of asynchronously creating and initializing resources before performing any work.

Some streams need to first asynchronously create resources before they can perform any work. Currently this is implemented in the different stream implementations which both makes things partly duplicated, more difficult, more error prone and often incorrect to some degree (e.g. 'open' and 'ready' are emitted after 'close').

This PR provides a standardized way of asynchronously constructing streams and handles the "pending" state that occurs until construction has either completed or failed.

This will allow further simplification and improved consistency for various stream implementations such as fs and net stream.

Passes the graceful-fs test suite.

This will make it possible to easily implement more complex stream such as e.g. fs streams:

const { Writable } = require('stream');
const fs = require('fs')

class WriteStream extends Writable {
  constructor (options) {
    options.autoDestroy = true;
    super(options);
  }
  _construct({ filename }, callback) {
    this.filename = filename;
    this.fd = null;
    fs.open(this.filename, (fd, err) => {
      if (err) {
        callback(err);
      } else {
        this.fd = fd;
        callback();
      }
    });
  }
  _write(chunk, encoding, callback) {
    fs.write(this.fd, chunk, callback);
  }
  _destroy(err, callback) {
    if (this.fd) {
      fs.close(this.fd, (er) => callback(er || err));
    } else {
      callback(err);
    }
  }
}

const { Readable } = require('stream');
const fs = require('fs')

class ReadStream extends Readable {
  constructor (options) {
    options.autoDestroy = true;
    super(options);
  }
  _construct({ filename }, callback) {
    this.filename = filename;
    this.fd = null;
    fs.open(this.filename, (fd, err) => {
      if (err) {
        callback(err);
      } else {
        this.fd = fd;
        callback();
      }
    });
  }
  _read(n) {
    const buf = Buffer.alloc(n);
    fs.read(this.fd, buf, 0, n, null, (err, bytesRead) => {
      if (err) {
        this.destroy(err);
      } else {
        this.push(bytesRead > 0 ? buf : null);
      }
    });
  }
  _destroy(err, callback) {
    if (this.fd) {
      fs.close(this.fd, (er) => callback(er || err));
    } else {
      callback(err);
    }
  }
}

Furthermore it makes it easier to e.g. add transforms into pipeline by inlining initialization:

const { Duplex } = require('stream');
const fs = require('fs')

stream.pipeline(
  fs.createReadStream('object.json')
    .setEncoding('utf-8'),
  new Duplex({
    construct (options, callback) {
      this.data = '';
      callback();
    },
    transform (chunk, encoding, callback) {
      this.data += chunk;
      callback();
    },
    flush(callback) {
      try {
        // Make sure is valid json.
        JSON.parse(this.data);
        this.push(this.data);
      } catch (err) {
        callback(err);
      }
    }
  }),
  fs.createWriteStream('valid-object.json')
);

Semver

I think this should be semver-major.

fs: Emit some previously synchronous errors asynchronously. See updated test. Bug fix.
fs: open() is removed but it still works if monkey-patched.
fs: Don't emit 'close' twice if emitClose: true.
fs: stream.fd is nulled during destroy instead of final. See updated test.

Otherwise, this should be pretty non breaking as far as I can tell. graceful-fs tests pass and there are tests for compat.

The changes are mostly opt-in through the construct method, except for the previously listed updates to fs streams.

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

Refs: #29314, #23133

NOTE TO SELF: After merge look into:

Remove emitClose and make 'close' behaviour align with streams.
Deprecate close(cb) in favour of end(cb). Make close(cb) call end(cb).
https://github.com/nxtedition/node/pull/new/stream-sync-final

@Fishrock123 I just started looking a bit into Bob streams and I'm curious about your take on this.

Can we use a different event name? I recognize fs.ReadStream/fs.WriteStream already use it, but by using this at the stream.Readable/stream.Writable level it can cause disruption in userland modules that may already use this event name (I'm speaking from experience here as ssh2-streams has used 'ready' since 2016 -- long before even fs started using it).

Can we use a different event name?

Sure, though I'm not sure what that could be, https://www.thesaurus.com/browse/ready?s=t.

'primed'?
'init'?

rebased

@addaleax

@mcollina is it an option to use Symbols as event "names" to avoid collisions?

e.g.

const r = new Readable();
r
 .on(Readable.READY, () => {})
 .on(Readable.DATA, (buf) => {})
 .on(Readable.ERROR, (err) => {});

Probably a too big of a change... just a possibly interesting idea...

I'm not sure what would be a better name that reduces the risk of clashing with userland. Perhaps an added prefix of some kind might help with that, like 'stream-init' or similar. It might seem redundant but it would help differentiate from someone already emitting 'init', as I think that would be more common than 'stream-init'.

I’ve tagged semver-major out of cautiousness because it’s a significant refactor.

@mcollina do you have any feedback or preference regarding @mscdex's concern?

I agree with @mscdex concern, we need to find a different name.

This needs a rebase.

@nodejs/streams @nodejs/tsc This PR have been stale for a while. Maybe another round of reviews might be appropriate to ensure we are all comfortable with the changes?

I have one thing I wanted to point out. Given the current implementation _construct must succeed eventually and there is no way to abort it since _destroy will wait for it by design. I don't think this is a problem just wanted to point it out in case anyone else see a problem with this.

Also, for those returning to this PR. The biggest change since the last round of reviews is that _construct is invoked in the next tick after the constructor, instead of directly at the end of the constructor.

Why the addition of the empty file, 'does not exist' ?

Why the addition of the empty file, 'does not exist' ?

Mistake. Removed.

There seems to be a consistent perf regression after multiple runs for Duplex creation:

streams/creation.js kind='duplex' n=50000000               ***     -8.15 %       ±1.76% ±2.34%  ±3.04%

There seems to be a consistent perf regression after multiple runs for Duplex creation:

This duplex creation regression seems to be re-occuring. Anyway, I think this is an unfortunate but acceptable regression given what this PR does.

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

Provide a standardized way of asynchronously creating and initializing resources before performing any work. Refs: nodejs#29314

Refs: nodejs#23133

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

The async construction feature is def nice and tricky to impl atm.
Do I read this correctly that no event is added after construction now in the latest draft?
Name collisions with that event would be my main concern with userland streams so if no event then I'm soft +1

Do I read this correctly that no event is added after construction now in the latest draft?

We do add an event but it's a Symbol to avoid collisions. Only used internally to keep the implementation simple.

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

@addaleax @lpinca I assume you are still LGTM?

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2386/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2387/

Yes 👍

@mcollina I'm getting a suspicious CITGM failures on pino. Any insight? https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2386/nodes=rhel7-s390x/testReport/(root)/citgm/pino_v6_3_0/ Is this a pino issue or issue with this PR?

Pino is ok with the change. A test fail because of another deprecation.

Landed in 9949a2e...54b36e4

arm-fanned has been offline for a while now and it's back on and should be working again but this PR is causing a perma-fail that wasn't caught because it was merged while they were offline.

17:42:04 not ok 583 parallel/test-fs-stream-construct
17:42:04   ---
17:42:04   duration_ms: 240.87
17:42:04   severity: fail
17:42:04   exitcode: -15
17:42:04   stack: |-
17:42:04     timeout
17:42:04   ...

Sorry @ronag, would you mind taking a look?

I haven't had a look at what this test does but a brief scan suggests that a timeout is probably odd so maybe the slower platforms are catching a genuine bug? If not, either use the platformTimeout() to adjust timings in this test for slower machines or maybe even skip it entirely if it can't be avoided.

I haven't had a look at what this test does but a brief scan suggests that a timeout is probably odd so maybe the slower platforms are catching a genuine bug? If not, either use the platformTimeout() to adjust timings in this test for slower machines or maybe even skip it entirely if it can't be avoided.

There is no timeout. It's probably the test not completing for whatever reason.

@rvagg Do you have a machine I can access for reproducing?

@ronag you can request access to a machine in https://github.com/nodejs/build/issues/new

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 22, 2019

ronag mentioned this pull request Sep 22, 2019

stream: construct nxtedition/node#2

Closed

4 tasks

ronag changed the title stream: provide a _construct endpoint for async stream initialization. stream: construct Sep 22, 2019

ronag force-pushed the stream-construct branch from 3ce4d2c to 37f1ab0 Compare September 22, 2019 21:05

ronag force-pushed the stream-construct branch 6 times, most recently from 10bf659 to 879c459 Compare September 22, 2019 21:24

ronag force-pushed the stream-construct branch from 879c459 to f25db5e Compare September 23, 2019 09:56

ronag force-pushed the stream-construct branch from f25db5e to a39af0c Compare September 23, 2019 09:57

ronag force-pushed the stream-construct branch 3 times, most recently from 4188668 to 2a16612 Compare September 23, 2019 11:40

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 24, 2019

mcollina reviewed Sep 24, 2019

View reviewed changes

Comment thread lib/internal/fs/streams.js Outdated

Comment thread lib/_stream_writable.js Outdated

Comment thread lib/_stream_writable.js Outdated

ronag mentioned this pull request Sep 24, 2019

[x] http: client destroy stream #29192

Closed

4 tasks

ronag force-pushed the stream-construct branch from 2a16612 to 250b78b Compare October 4, 2019 06:36

ronag added 2 commits May 25, 2020 10:21

stream: construct

2fe1044

Provide a standardized way of asynchronously creating and initializing resources before performing any work. Refs: nodejs#29314

fs: reimplement read and write streams using stream.construct

f270832

Refs: nodejs#23133

mcollina approved these changes May 26, 2020

View reviewed changes

addaleax approved these changes May 26, 2020

View reviewed changes

richardlau mentioned this pull request May 28, 2020

flaky test-fs-stream-construct test #33607

Closed

antsmartian mentioned this pull request May 29, 2020

Use stream._construct #33586

Closed

5 tasks

richardlau mentioned this pull request Jun 8, 2020

parallel/test-fs-stream-construct reliably fails on fanned arm build #33796

Closed

mcollina mentioned this pull request Jun 26, 2020

stream.pipeline swallowing errors when read stream is empty #24517

Closed

BethGriggs mentioned this pull request Sep 1, 2020

2020-10-20, Version 15.0.0 (Current) #35014

Merged

brendankenny mentioned this pull request Nov 16, 2020

throw on reading undefined file path wardpeet/brfs#1

Merged

Ayase-252 mentioned this pull request Jun 24, 2021

Custom Readable Stream nodejs/help#3424

Closed

stream: construct by ronag · Pull Request #29656 · nodejs/node · GitHub

Uh oh!

Uh oh!

stream: construct - #29656

stream: construct#29656ronag wants to merge 2 commits intonodejs:masternodejs/node:masterfrom nxtedition:stream-constructnxtedition/node:stream-constructCopy head branch name to clipboard

Conversation

ronag commented Sep 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Semver

Checklist

Uh oh!

ronag commented Sep 22, 2019

Uh oh!

mscdex commented Sep 22, 2019

Uh oh!

ronag commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Sep 23, 2019

Uh oh!

ronag commented Sep 23, 2019

Uh oh!

ronag commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Sep 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronag commented Sep 24, 2019

Uh oh!

mcollina commented Sep 24, 2019

Uh oh!

Trott commented Oct 1, 2019

Uh oh!

ronag commented Apr 28, 2020

Uh oh!

mscdex commented Apr 28, 2020

Uh oh!

ronag commented Apr 28, 2020

Uh oh!

mscdex commented Apr 29, 2020

Uh oh!

ronag commented Apr 29, 2020

Uh oh!

nodejs-github-bot commented May 18, 2020

Uh oh!

nodejs-github-bot commented May 25, 2020

Uh oh!

mafintosh commented May 26, 2020

Uh oh!

ronag commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented May 26, 2020 • edited by ronag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented May 26, 2020

Uh oh!

ronag commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented May 27, 2020

Uh oh!

ronag commented May 27, 2020

Uh oh!

rvagg commented Jun 8, 2020

Uh oh!

ronag commented Jun 8, 2020

Uh oh!

stream: construct#29656
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:stream-construct

ronag commented Sep 22, 2019 •
edited
Loading

ronag commented Sep 23, 2019 •
edited
Loading

ronag commented Sep 23, 2019 •
edited
Loading

mscdex commented Sep 23, 2019 •
edited
Loading

ronag commented May 26, 2020 •
edited
Loading

nodejs-github-bot commented May 26, 2020 •
edited by ronag
Loading

ronag commented May 26, 2020 •
edited
Loading

ronag commented May 26, 2020 •
edited
Loading