Skip to content
New issue

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

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

Already on GitHub? # 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 !

@indutny indutny closed this Mar 22, 2015
@indutny indutny deleted the fix/gh-1068 branch March 22, 2015 02:51
@rvagg rvagg mentioned this pull request Mar 22, 2015
@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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants