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

streams: non-writable Duplex is writable (more annoyance than bug) #34374

Closed
jasnell opened this issue Jul 15, 2020 · 7 comments
Closed

streams: non-writable Duplex is writable (more annoyance than bug) #34374

jasnell opened this issue Jul 15, 2020 · 7 comments
Assignees
Labels
discuss Issues opened for discussions and feedbacks. stream Issues and PRs related to the stream subsystem.

Comments

@jasnell
Copy link
Member

jasnell commented Jul 15, 2020

@mcollina @ronag @nodejs/streams

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

const d = new Duplex({
  writable: false,
  write(chunk, encoding, cb) {
    console.log(chunk.toString());
    cb();
  }
});

console.log(d.writable);   // false! as expected...

d.write('darn it');  // prints, 'darn it' and returns true

This isn't a bug since it's been like this forever but the behavior is really counter intuitive, especially since after calling d.end() and then doing a d.write() we get a proper write after end error. It makes implementing a custom Duplex (e.g. QuicStream) more difficult because of the additional checks that need to be made to ensure that even tho the Duplex isn't writable no-one is writing to it.

Not sure what the fix is immediately but wanted to discuss it first.

The ideal behavior, I would think, is an error similar to write after end if write() is called on a non-writable Duplex.

@jasnell jasnell added stream Issues and PRs related to the stream subsystem. discuss Issues opened for discussions and feedbacks. labels Jul 15, 2020
@jasnell
Copy link
Member Author

jasnell commented Jul 15, 2020

Also, just to be thorough, the same issue exists with readable: false and the readable side. Specifically:

const d = new stream.Duplex({
  readable: false,
  read() {
    this.push(Buffer.from('abc'));
    this.push(null);
  }
});

console.log(d.readable);  // false

d.setEncoding('utf8');
d.on('data', console.log);  // prints abc

@ronag
Copy link
Member

ronag commented Jul 15, 2020

What would you like to happen? Error if write/push to a non writable/readable stream?

readable/writable false should be as if end()/push(null) has been called in the constructor but without emitting the associated events?

@preyunk
Copy link
Contributor

preyunk commented Jul 15, 2020

@ronag @jasnell
Correct me if I am wrong here, according to the readable.readable and writable.writable docs they must be used to check if it is safe to read/write on the stream. That's why setting writable and readable explicitly doesn't seem to work, maybe that's the reason we don't get the option to set writable and readable property as options while creating a new Duplex.
However if I am wrong, the ideal behavior that is throwing an error as proposed by @jasnell looks good.

@ronag
Copy link
Member

ronag commented Jul 15, 2020

Correct me if I am wrong here, according to the readable.readable and writable.writable docs they must be used to check if it is safe to read/write on the stream

No, they don't need to be checked. Calling it "safe" is maybe a bit misleading, but the following sentence does explain what it means in the context.

That's why setting writable and readable explicitly doesn't seem to work

It does "work". Depends on what you mean by "works". Though that usage is deprecated.

maybe that's the reason we don't get the option to set writable and readable property as options while creating a new Duplex.

That option actually does exist and should be used. It's just not documented, which we should fix.

@ronag
Copy link
Member

ronag commented Jul 15, 2020

@jasnell I'll investigate what we can do to improve this.

@ronag ronag self-assigned this Jul 15, 2020
@preyunk
Copy link
Contributor

preyunk commented Jul 15, 2020

It does "work". Depends on what you mean by "works". Though that usage is deprecated.

By "doesn't work" I meant that we are allowed to read/write on a non readable/writable Duplex

That option actually does exist and should be used. It's just not documented, which we should fix.

Should I open another issue regarding this? Also are there any more options other than writable and readable that must be included in the docs?

@ronag
Copy link
Member

ronag commented Jul 15, 2020

Should I open another issue regarding this?

Sure!

Also are there any more options other than writable and readable that must be included in the docs?

I don't think so.

ronag added a commit to nxtedition/node that referenced this issue Jan 5, 2021
If writable/readable has been explicitly disabled then using
a Duplex as writable/readable should fail.

Fixes: nodejs#34374
ronag added a commit to nxtedition/node that referenced this issue Jul 1, 2021
If writable/readable has been explicitly disabled then using
a Duplex as writable/readable should fail.

Fixes: nodejs#34374
ronag added a commit to nxtedition/node that referenced this issue Jul 1, 2021
If writable/readable has been explicitly disabled then using
a Duplex as writable/readable should fail.

Fixes: nodejs#34374
@ronag ronag closed this as completed in 954217a Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants