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

node: ensure that streams2 won't .end() stdin #1233

Closed
wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 21, 2015

Stdin is purely read-only stream. Although, net.Socket might be used
to create it if stdin is in fact a Pipe or TCP socket, the
stream.Duplex should not try to call .end() on it.

Fix: #1068

cc @rvagg

Stdin is purely read-only stream. Although, `net.Socket` might be used
to create it if stdin is in fact a Pipe or TCP socket, the
`stream.Duplex` should not try to call `.end()` on it.

Fix: nodejs#1068
@indutny
Copy link
Member Author

indutny commented Mar 21, 2015

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/359/ . I hope it'll help :)

@indutny
Copy link
Member Author

indutny commented Mar 21, 2015

Yay, build appears to be green (except one spurious failure)!

@@ -1 +1,2 @@
debugger;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, thanks! Fixed.

@@ -630,6 +630,8 @@
writable: false
});
}
// Ensure that Streams2 won't try to `.end()` the stream
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might rephrase this comment to make sure that stdin can't be .end()-ed, since we're on streams3 now.

@chrisdickinson
Copy link
Contributor

LGTM other than comment nit (which can be ignored if desired.)

indutny added a commit that referenced this pull request Mar 22, 2015
Stdin is purely read-only stream. Although, `net.Socket` might be used
to create it if stdin is in fact a Pipe or TCP socket, the
`stream.Duplex` should not try to call `.end()` on it.

Fix: #1068
PR-URL: #1233
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@indutny
Copy link
Member Author

indutny commented Mar 22, 2015

Thank you, landed in 9ae1a61 !

@bnoordhuis
Copy link
Member

I don't think this is a good fix. state.ended === true is co-opted to mean different things now. We'll need to introduce a state.reallyEnded property next.

@indutny
Copy link
Member Author

indutny commented Mar 22, 2015

@bnoordhuis not really, it means the same thing as it was before this patch.

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.

4 participants