Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: construct #2

Closed
wants to merge 1 commit into from
Closed

stream: construct #2

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 25, 2019

Refs: nodejs#29314

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
  • fs: Don't emit 'open' after destroy(). See updated test. Bug fix.
  • fs: Emit some previously synchronous errors asynchronously. See updated test. Bug fix.
  • fs: open() is removed but it still works if monkey-patched. Deprecation.

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

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).

@ronag ronag force-pushed the stream-construct branch from 51cc654 to 8c4868a Compare August 25, 2019 16:20
@ronag
Copy link
Member Author

ronag commented Aug 25, 2019

@benjamingr initial feedback welcome

@ronag ronag force-pushed the stream-construct branch 3 times, most recently from c5c9a52 to 31c9d54 Compare August 25, 2019 16:24
@ronag ronag changed the title stream: provide a _construct endpoint for async stream initialization. stream: standardize async initialization Aug 25, 2019
@ronag ronag force-pushed the stream-construct branch 4 times, most recently from 8b2f461 to d9c8f4c Compare August 25, 2019 16:37
@ronag ronag force-pushed the stream-construct branch 15 times, most recently from 212a00e to 3642d1a Compare August 25, 2019 21:26
@ronag
Copy link
Member Author

ronag commented Aug 25, 2019

Fully implemented in fs and passes all stream and fs tests.

@ronag ronag force-pushed the stream-construct branch from 3642d1a to 4ad179e Compare August 25, 2019 21:30
@ronag
Copy link
Member Author

ronag commented Aug 25, 2019

Implementing this in net is significantly more complicated. Probably better to do in a separate follow up PR?

@ronag ronag force-pushed the stream-construct branch 26 times, most recently from 8105c72 to ee8ccf7 Compare September 8, 2019 09:04
Some streams need to first asynchronously create a resources before
it can perform any work. Currently this is implemented in the different
stream implementations which both makes things more difficult and
error prone. Provide a standardized way of achieving this.
@ronag
Copy link
Member Author

ronag commented Sep 22, 2019

moved to nodejs#29656

@ronag ronag closed this Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant