Skip to content
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

test-process-active-wraps fails on windows #246

Closed
piscisaureus opened this issue Jan 7, 2015 · 5 comments · May be fixed by aliscco/alisco-node#232
Closed

test-process-active-wraps fails on windows #246

piscisaureus opened this issue Jan 7, 2015 · 5 comments · May be fixed by aliscco/alisco-node#232
Milestone

Comments

@piscisaureus
Copy link
Contributor

Tracked in node: nodejs/node-v0.x-archive#8986

According to @cjihrig:

The problem seems to be the removal of this logic.

The problem is likely due to the DNS resolution that's now needed, so net.js postpones TCPWrap creation, now doing it on the same loop turn in which the assertion is done.

@piscisaureus piscisaureus added this to the v1.0.0 milestone Jan 7, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Jan 7, 2015

nodejs/node-v0.x-archive#8989 is a proposed fix.

@piscisaureus
Copy link
Contributor Author

@cjihrig Is there any motivation for that revert, other than just to make the test pass.

It's quite easy to make the test pass. Just do:
setTimeout(function() { setInterval(function() { ... do the check here ... }); }, 10);

That gives the event loop enough time to clean up the TCPWrap object.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 7, 2015

@piscisaureus yes, the revert was done to get everything back to passing. I originally tried adding a setTimeout() of 5 seconds around the setImmediate(). The test still failed - the server handle was cleaned up, but the timeout was lingering.

The test does pass if I add a setTimeout() AND a setImmediate() around the original setImmediate(), but that just seems so hacky.

@piscisaureus
Copy link
Contributor Author

@cjihrig
If that (now reverted) patch made node better, put it back and fix the test in the hacky way.
The test is quite questionable - the _getActiveHandles() and _getActiveRequests() APIs are there for debugging only, node makes no guarantees about their contents. It just happens to be that Timer handles end up in that list and Immediate handles don't. And that on windows the TCPWrap might live a bit longer than on unix.

cjihrig added a commit that referenced this issue Jan 8, 2015
b636ba8 broke this test, because it now takes a loop iteration or two
to resolve the loopback address. That consequence is that the TCPWrap
handle that we *don't* want to see is created a bit later, and also
destroyed later, so when we assert that the active handle list is empty
the TCPWrap object is still "busy" being closed.

Wait one extra loop iteration before checking there are no more active
handles. This allows name resolution and clean-up to finish before the
assertion.

BUG: #246
PR-URL: nodejs/node-v0.x-archive#8998
Reviewed-By: Bert Belder <[email protected]>
@piscisaureus
Copy link
Contributor Author

Fixed in b5c9dcb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants