-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
deps: update libuv to 1.2.0 #237
Conversation
Probably unrelated but:
|
lgtm |
PR-URL: nodejs#237 Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
LGTM. (and tests pass here on OSX, FWIW) |
a877b44
to
eaed2a1
Compare
@piscisaureus Curiously enough, the same test is failing on the CI. I've been going over the changes in libuv but I don't see an obvious culprit. |
@piscisaureus I let the CI do some ad hoc bisecting and it looks like the regression was introduced in commit 94e1475, the joyent/v0.12 merge. Maybe you can run a |
I'll look into this too. I recently changed that test in b636ba8. It works fine on OS X, but fails on Windows. |
@cjihrig It's probably unrelated but there was a merge conflict around line 840 in lib/net.js when I did the merge. It's possible I screwed up conflict resolution although it doesn't look like it (and like you say, the test passes on other platforms.) |
The problem seems to be the removal of this logic. @piscisaureus any clue why that would leave an extra handle around on Windows? It was removed to try to eliminate hard coded IPv4 addresses. |
R=@piscisaureus or @saghul?