Skip to content

tty: do not add shutdown method to handle#1073

Closed
indutny wants to merge 1 commit intonodejs:v1.xfrom
indutny:fix/gh-1068
Closed

tty: do not add shutdown method to handle#1073
indutny wants to merge 1 commit intonodejs:v1.xfrom
indutny:fix/gh-1068

Conversation

@indutny
Copy link
Copy Markdown
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
Copy Markdown
Member Author

indutny commented Mar 5, 2015

@bnoordhuis please review

@cjihrig
Copy link
Copy Markdown
Contributor

cjihrig commented Mar 5, 2015

LGTM

@indutny
Copy link
Copy Markdown
Member Author

indutny commented Mar 5, 2015

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

@indutny
Copy link
Copy Markdown
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
Copy Markdown
Member Author

indutny commented Mar 5, 2015

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

@indutny
Copy link
Copy Markdown
Member Author

indutny commented Mar 5, 2015

Erm, better @piscisaureus

@qfox
Copy link
Copy Markdown

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
Comment thread src/stream_base.h
Copy link
Copy Markdown
Member

@bnoordhuis bnoordhuis Mar 5, 2015

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
Copy Markdown
Member Author

@indutny indutny Mar 5, 2015

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
Copy Markdown
Member

@bnoordhuis bnoordhuis Mar 5, 2015

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
Copy Markdown
Member Author

indutny commented Mar 5, 2015

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

@qfox
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Member Author

indutny commented Mar 5, 2015

@bnoordhuis LGTY?

@bnoordhuis
Copy link
Copy Markdown
Member

bnoordhuis commented Mar 5, 2015

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Contributor

Fishrock123 commented Mar 17, 2015

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
Copy Markdown

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
Copy Markdown
Contributor

Fishrock123 commented Mar 17, 2015

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

@qfox
Copy link
Copy Markdown

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