Windows socket and named pipe servers don't work in main process

[mmastrac]

I've been tracking down a bug in a program that works in OSX but fails mysteriously in Windows. I've pared down a minimal repro case to the net.createConnection / net.createServer APIs w/ a named pipe failing when run in the main atom process.

If you specify ATOM_SHELL_INTERNAL_RUN_AS_NODE=1 for both ends of the test it will function as you'd expect. If either one (or both) are ATOM_SHELL_INTERNAL_RUN_AS_NODE=0, the repro will hang in the middle of the test and fail to continue the ping/pong cycle.

Reproduction steps (tested on Win7/32 w/electron-v0.28.0-win32-ia32):

• Save the snippet below as repro.js
• Open two Windows command consoles
• In the first one, run electron.exe repro.js
• In the second one, run electron.exe repro.js client

What happens:

• The ping/pong only runs a few cycles back and forth, then stops. It may occasionally continue after a pause of 30-60s.
  

What I expect to happen:

• The ping/pong continues indefinitely
  

var net = require('net');

var addr = '\\\\?\\pipe\\testpipe';

var counter = 0;

if (process.argv[2] == 'client') {
    var client = net.createConnection({ path: addr }, function() {
        console.log("Connected to named pipe as client");
        client.on('data', function(data) {
            console.log("rec'd: " + data);
            setTimeout(function() {
                client.write("ping " + counter++);
            }, 500);
        });

        client.write('ping!');
    }.bind(this));
} else {
    net.createServer(function(client) {
        console.log("Connected to named pipe as server");
        client.on('data', function(data) {
            console.log("rec'd: " + data);
            setTimeout(function() {
                client.write("pong " + counter++);
            }, 500);
        });
    }.bind(this)).listen(addr);
}

[mmastrac]

It looks like this fails for TCP servers as well. Updating bug title to match.

var net = require('net');

var counter = 0;

if (process.argv[2] == 'client') {
    var client = net.createConnection({ host: '127.0.0.1', port: 12345 }, function() {
        console.log("Connected to named pipe as client");
        client.on('data', function(data) {
            console.log("rec'd: " + data);
            setTimeout(function() {
                client.write("ping " + counter++);
            }, 500);
        });

        client.write('ping!');
    }.bind(this));
} else {
    net.createServer(function(client) {
        console.log("Connected to named pipe as server");
        client.on('data', function(data) {
            console.log("rec'd: " + data);
            setTimeout(function() {
                client.write("pong " + counter++);
            }, 500);
        });
    }.bind(this)).listen(12345, '127.0.0.1');
}

[zcbenz]

Seems like a bug of Node.js integration.

[mmastrac]

This patch appears to fix the problem, but I don't know enough about how the integration works to say if it has other consequences.

The interesting thing is that this now looks a lot like the Mac and Linux node integrations. Note that uv_backend_timeout might return -1 here. I noticed that the Linux node integration doesn't handle a -1, but the Mac one does. I think that the appropriate action on Windows would be to treat a -1 as the Windows constant 'INFINITE'.

If you think this is the right approach, I can turn it into a pull request:

diff --git a/atom/common/node_bindings_win.cc b/atom/common/node_bindings_win.cc
index b1ecaa5..21caabc 100644
--- a/atom/common/node_bindings_win.cc
+++ b/atom/common/node_bindings_win.cc
@@ -22,37 +22,26 @@ NodeBindingsWin::~NodeBindingsWin() {
 }

 void NodeBindingsWin::PollEvents() {
-  // Unlike Unix, in which we can just rely on one backend fd to determine
-  // whether we should iterate libuv loop, on Window, IOCP is just one part
-  // of the libuv loop, we should also check whether we have other types of
-  // events.
-  bool block = uv_loop_->idle_handles == NULL &&
-               uv_loop_->pending_reqs_tail == NULL &&
-               uv_loop_->endgame_handles == NULL &&
-               !uv_loop_->stop_flag &&
-               (uv_loop_->active_handles > 0 ||
-                !QUEUE_EMPTY(&uv_loop_->active_reqs));
-
-  // When there is no other types of events, we block on the IOCP.
-  if (block) {
-    DWORD bytes, timeout;
-    ULONG_PTR key;
-    OVERLAPPED* overlapped;
-
-    timeout = uv_backend_timeout(uv_loop_);
-    GetQueuedCompletionStatus(uv_loop_->iocp,
-                              &bytes,
-                              &key,
-                              &overlapped,
-                              timeout);
-
-    // Give the event back so libuv can deal with it.
-    if (overlapped != NULL)
-      PostQueuedCompletionStatus(uv_loop_->iocp,
-                                 bytes,
-                                 key,
-                                 overlapped);
-  }
+  // If there are other kinds of events pending, uv_backend_timeout will instruct us
+  // not to wait.
+  DWORD bytes, timeout;
+  ULONG_PTR key;
+  OVERLAPPED* overlapped;
+
+  timeout = uv_backend_timeout(uv_loop_);
+
+  GetQueuedCompletionStatus(uv_loop_->iocp,
+                            &bytes,
+                            &key,
+                            &overlapped,
+                            timeout);
+
+  // Give the event back so libuv can deal with it.
+  if (overlapped != NULL)
+    PostQueuedCompletionStatus(uv_loop_->iocp,
+                               bytes,
+                               key,
+                               overlapped);
 }

 // static

[mmastrac]

@zcbenz Quick ping -- what do you think of the patch I've suggested here?

[zcbenz]

@mmastrac Doing that used to cause timers not working when we used Node 0.10.x in old days, but it seems to work perfectly now. I'll include this patch in next release and see if it breaks things.

[mmastrac]

@zcbenz thanks!

[zcbenz]

@mmastrac Can you send a PR for it?

[mmastrac]

@zcbenz PR #2545 created

[zcbenz]

Closed by #2545.

[cjb]

@mmastrac Hi! I'm afraid this is still broken here on Windows 10 Home, using the same testcase:

var net = require('net')
var client = net.createConnection({ path: '\\\\.\\pipe\\mypipe', function () { console.log('connected') })

This works ("connected" is printed) when run as a script executed by node v5.1.1, but fails (hangs forever) when executed by Electron v0.36.0, which includes your fix. Looking at the client object shows it's hung in connecting: true.

Anything I can do to help debug?

[mmastrac]

@cjb interesting. I haven't had a chance to test this stuff lately but it's possible something else broke. Are you set up to compile the latest from source?

What does the output look like from my test case exactly?

[zcbenz]

Reopening as this problem is happening again per microsoft/vscode#142786.