unix,stream: fix getting the correct fd for a handle#6838
Conversation
nodejs-github-bot added the c++ 
kzc commented May 18, 2016
This is the test case that does not work as expected with node 4.x and 5.x on Mac: |
cjihrig commented May 18, 2016
Can we translate that into an actual test to prevent regressions? |
saghul commented May 18, 2016
Also, bonus nachos: we're no longer relying on libuv internals to get the fd. |
saghul commented May 18, 2016
I'm not sure. It would be a OSX only test, which only works if we replaced the fd in the select trick. This could change over time if Apple fixes kqueue to support other fds. An alternate title would be along the lines of: "use official uv API instead of internal structures". |
kzc commented May 18, 2016
Because it's a tty test it's more difficult to automate. Spawning, piping and redirecting change the code path within node and libuv pertaining to stdout and stderr. |
cjihrig commented May 18, 2016
OK, well I did verify that the change worked on my OS X box, so LGTM. |
jasnell commented May 18, 2016
Nice. LGTM. A test would be good but can definitely understand the constraints. |
src/stream_wrap.cc Outdated
There was a problem hiding this comment.
I think you might as well remove the #if !defined(_WIN32) guard now.
There was a problem hiding this comment.
On Windows that's a HANDLE*, and while it can be cast to an int because only 32 bits are used AFAIS, I wasn't sure if we wanted to expose that.
There was a problem hiding this comment.
Good point. Let's leave it like this, then.
kzc commented May 18, 2016
FWIW, dtruss on Mac showed that node 4.x and 5.x |
bnoordhuis commented May 18, 2016
LGTM |
On OSX it's possible that the fd is replaced, so use the proper libuv API to get the correct fd. PR-URL: nodejs#6753 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
bnoordhuis commented May 20, 2016
@saghul I didn't realize it at the time but we have similar logic in |
saghul commented May 20, 2016
Oh, me neither. I'll make a PR later today if nobody beats me to it.
|
Refs: nodejs#6838 PR-URL: nodejs#6908 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 commented May 25, 2016
MylesBorins commented May 26, 2016
@Fishrock123 patch applies cleanly to |
Refs: nodejs#6838 PR-URL: nodejs#6908 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins commented Jun 1, 2016
@saghul is this at all specific to v1.9.0? |
Refs: #6838 PR-URL: #6908 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: nodejs#6838 PR-URL: nodejs#6908 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #6838 PR-URL: #6908 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #6838 PR-URL: #6908 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #6838 PR-URL: #6908 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #6838 PR-URL: #6908 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: #6838 PR-URL: #6908 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
9 participants
Checklist
Affected core subsystem(s)
stream
Description of change
On OSX it's possible that the fd is replaced, so use the proper libuv
API to get the correct fd.