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.

Sign up for GitHub

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

Jump to bottom

tty: do not add shutdown method to handle #1073

Closed
wants to merge 1 commit into from
Closed

tty: do not add shutdown method to handle #1073

wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 5, 2015

UV_TTY does not support uv_shutdown() so adding this method in
StreamBase will cause an abort() in C land.

Fix: #1068

@indutny
tty: do not add shutdown method to handle
32933ba 
UV_TTY does not support `uv_shutdown()` so adding this method in
StreamBase will cause an `abort()` in C land.

Fix: nodejs#1068
@indutny indutny mentioned this pull request Mar 5, 2015
Closed
@indutny
Copy link
Member Author

indutny commented Mar 5, 2015

@bnoordhuis please review

@cjihrig
Copy link
Contributor

cjihrig commented Mar 5, 2015

LGTM

@indutny
Copy link
Member Author

indutny commented Mar 5, 2015

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/244/

@indutny
Copy link
Member Author

indutny commented Mar 5, 2015

Nah, I'm still getting a error on windows. @zxqfox was it working on windows on initial io.js release?

@indutny
Copy link
Member Author

indutny commented Mar 5, 2015

Hm... this test appears to be passing for me on windows machine. cc @rvagg

@indutny
Copy link
Member Author

indutny commented Mar 5, 2015

Erm, better @piscisaureus

@qfox
Copy link

qfox commented Mar 5, 2015

@indutny I can't check this for now but as I remember — yes.

bnoordhuis
bnoordhuis reviewed Mar 5, 2015
View reviewed changes
src/stream_base.h
@@ -160,7 +160,8 @@ class StreamBase : public StreamResource {
public:
enum Flags {
kFlagNone = 0x0,
kFlagHasWritev = 0x1
kFlagHasWritev = 0x1,
kFlagNoShutdown = 0x2
Copy link
Member

Choose a reason for hiding this comment

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

You're probably doing this so you don't have to update too much code but shouldn't it be opt-in with kFlagHasShutdown?

Copy link
Member Author

Choose a reason for hiding this comment

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

TTY seems to be in minority here, as other do support it. I guess I'd rather do it this way if this is not something serious for you?

Copy link
Member

Choose a reason for hiding this comment

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

Not serious, no, just a little incongruent.

@indutny
Copy link
Member Author

indutny commented Mar 5, 2015

Guess this is some mingw thing, landing. Thank you!

@qfox
Copy link

qfox commented Mar 5, 2015

@indutny I've just installed 1.3.0 and iojs -e 'process.stdin.emit("end");' doesn't produce any errors neither in mingw32, nor in cmd.

@indutny
Copy link
Member Author

indutny commented Mar 5, 2015

Ok, I can reproduce this thing without this patch, and can't do it with this patch. Not sure why CI is not happy :(

@indutny
Copy link
Member Author

indutny commented Mar 5, 2015

@bnoordhuis LGTY?

@bnoordhuis
Copy link
Member

LGTM

indutny added a commit that referenced this pull request Mar 5, 2015
@indutny
tty: do not add shutdown method to handle
3446ff4 
UV_TTY does not support `uv_shutdown()` so adding this method in
StreamBase will cause an `abort()` in C land.

Fix: #1068
PR-URL: #1073
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@indutny
Copy link
Member Author

indutny commented Mar 5, 2015

Thanks, landed in 3446ff4!

@indutny indutny closed this Mar 5, 2015
@indutny indutny deleted the fix/gh-1068 branch March 5, 2015 18:40
@qfox
Copy link

qfox commented Mar 5, 2015

Thanks! Will wait for build ;-) I have a couple of bugs on windows... But need fresh build.

@rvagg rvagg mentioned this pull request Mar 5, 2015
Closed
@Fishrock123 Fishrock123 mentioned this pull request Mar 13, 2015
Closed
@Fishrock123
Copy link
Contributor

The test included in here is the sole remaining persistently failing windows test in #1005

(see also: #1068 (comment))

=== release test-regress-GH-io-1068 ===
Path: parallel/test-regress-GH-io-1068
events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: shutdown EPIPE
    at exports._errnoException (util.js:734:11)
    at Socket.onSocketFinish (net.js:218:26)
    at emitNone (events.js:67:13)
    at Socket.emit (events.js:163:7)
    at finishMaybe (_stream_writable.js:477:14)
    at endWritable (_stream_writable.js:486:3)
    at Socket.Writable.end (_stream_writable.js:452:5)
    at Socket.end (net.js:393:31)
    at process._tickCallback (node.js:349:13)
    at Function.Module.runMain (module.js:453:11)
Command: c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\Release\iojs.exe c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\test\parallel\test-regress-GH-io-1068.js

@qfox
Copy link

qfox commented Mar 17, 2015

@Fishrock123 Did you mean that you getting this error with 1.5? Or just in testing env on CI?

@Fishrock123
Copy link
Contributor

My windows partition isn't setup for dev stuff; that's on the CI. (and io.js latest: 1.5.1)

@qfox
Copy link

qfox commented Mar 17, 2015

@Fishrock123 Thanks. Just probably something is stuck on the CI side. Not sure atm ;-(

@brendanashworth brendanashworth added the tty Issues and PRs related to the tty subsystem. label Aug 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
tty Issues and PRs related to the tty subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@indutny @cjihrig @qfox @bnoordhuis @Fishrock123 @brendanashworth