-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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.
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
udp: use libuv API to get file descriptor #6908
Conversation
LGTM |
LGTM... would be ideal if there was some way to actually test this tho so we don't get regressions later |
LGTM |
@jasnell I'm open to suggestions, but how do we test that the way to obtain that value has changed in C++ ? FWIW, before this change Node was relying on libuv internals, which can be seen as "broken" already, because we could've changed it without notice. |
;-) I didn't say I had any good suggestions for a test ;-) I'm fine with this landing as is, but if someone can come up with a way of testing then +1 |
Refs: nodejs#6838 PR-URL: nodejs#6908 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: nodejs#6838 PR-URL: nodejs#6908 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@saghul is it safe to assume that this will be neccessary if we want to backport libuv 1.9? Not entirely sure if this should land or not either way |
I'd land it because it poses 0 risk and it makes process.std*._handle.fd
|
Refs: nodejs#6838 PR-URL: nodejs#6908 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: #6838 (comment)