test-process-active-wraps.js fails on windows

[misterdjules]

It seems that commit b636ba8 introduced the following regression on Windows:

>.\Release\node.exe test\simple\test-process-active-
wraps.js

assert.js:98
  throw new assert.AssertionError({
        ^
AssertionError: 1 == 0
    at Immediate._onImmediate (c:\Users\jgilli\dev\node-bis\test\simple\test-pro
cess-active-wraps.js:69:16)
    at processImmediate [as _immediateCallback] (timers.js:340:17)

>

@cjihrig Would you mind taking a look?

[cjihrig]

The failure is apparently related to the removal of these lines. I'm not sure why the extra handle hangs around on Windows when a DNS lookup occurs, but I will restore the relevant code as a bandaid.

[misterdjules]

@cjihrig What happens on Windows when a DNS lookup occurs is that both close events are emitted (one for the server, one for the client), even though the server's tcp handle didn't really close.

Since the client performs a DNS lookup with process.nextTick, the call to destroy can really close the underlying handle since there's no request pending and the close event can be emitted on the client.

For the server, the close event is emitted regardless of the underlying TCP handle being closed or not. Instead of HandleWrap::Close emitting the close event when the underlying handle is really closed, _emitCloseIfDrained emits it with process.nextTick because no connection has been accepted.

Thus, on Windows, the close event listener is called twice earlier than without the change . The check for active handles still finds one active and asserts.

On UNIX, the handle is removed from the active handles immediately, so the problem doesn't happen. On Windows, it is not removed immediately because there are still requests pending.

One way to fix this issue would be to actually emit the close event on the server when the underlying handle is really closed. Something like:

diff --git a/lib/net.js b/lib/net.js
index 0ece1b0..35e3440 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -1353,8 +1353,10 @@ Server.prototype.close = function(cb) {
   }

   if (this._handle) {
-    this._handle.close();
-    this._handle = null;
+    var self = this;
+    this._handle.close(function onClose() {
+      self.emit('close');
+      this._handle = null;
+    });
   }

   if (this._usingSlaves) {

However, the documentation states that the close event is emitted on the server:

when the server closes. Note that if connections exist, this event is not emitted until all connections
are ended.
which doesn't necessarily mean that it's emitted after the underlying TCP handle is closed.

In this interpretation, it makes sense not to wait for the underlying handle to close before emitting the close event.

Thus, instead of reverting some part of the original change, maybe we could make the test not to make that assumption anymore, and check for active handles in a setTimeout callback after some arbitrary duration?

[cjihrig]

@misterdjules thanks for looking into this. I already closed the partial revert PR. I think I came up with a solution after talking with @piscisaureus on nodejs/node#246. I plan to PR it tomorrow morning.

[cjihrig]

Closed via 1fad373