unix,stream: fix getting the correct fd for a handle #6838
Conversation
nodejs-github-bot
added
the
c++
|
This is the test case that does not work as expected with node 4.x and 5.x on Mac: |
|
Can we translate that into an actual test to prevent regressions? |
|
Also, bonus nachos: we're no longer relying on libuv internals to get the fd. |
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". |
|
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. |
|
OK, well I did verify that the change worked on my OS X box, so LGTM. |
|
Nice. LGTM. A test would be good but can definitely understand the constraints. |
| @@ -86,7 +86,7 @@ int StreamWrap::GetFD() { | |||
| int fd = -1; | |||
| #if !defined(_WIN32) | |||
| if (stream() != nullptr) | |||
| fd = stream()->io_watcher.fd; | |||
| uv_fileno(reinterpret_cast<uv_handle_t*>(stream()), &fd); | |||
| #endif | |||
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.
|
FWIW, dtruss on Mac showed that node 4.x and 5.x |
|
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]>
|
@saghul I didn't realize it at the time but we have similar logic in |
|
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 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]>
|
@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]>
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.