Skip to content

Conversation

@bnoordhuis
Copy link
Member

@piscisaureus
Copy link
Contributor

Probably unrelated but:

=== release test-process-active-wraps === Path: parallel/test-process-active-wraps assert.js:100 throw new assert.AssertionError({^ AssertionError: 1 == 0 at Immediate._onImmediate (D:\iojs\test\parallel\test-process-active-wraps.js:69:16) at processImmediate [as _immediateCallback] (timers.js:342:17) 

@piscisaureus
Copy link
Contributor

lgtm

PR-URL: nodejs#237 Reviewed-By: Bert Belder <[email protected]> Reviewed-By: Saúl Ibarra Corretgé <[email protected]>
@saghul
Copy link
Member

LGTM. (and tests pass here on OSX, FWIW)

@bnoordhuisbnoordhuis merged commit eaed2a1 into nodejs:v1.xJan 5, 2015
@bnoordhuisbnoordhuis deleted the upgrade-libuv branch January 5, 2015 22:54
@bnoordhuis
Copy link
MemberAuthor

@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.

@bnoordhuis
Copy link
MemberAuthor

@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 git bisect locally to track down the offending commit?

@cjihrig
Copy link
Contributor

I'll look into this too. I recently changed that test in b636ba8. It works fine on OS X, but fails on Windows.

@bnoordhuis
Copy link
MemberAuthor

@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.)

@cjihrig
Copy link
Contributor

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.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@bnoordhuis@piscisaureus@saghul@cjihrig