test: fix flaky test-net-socket-local-address

[Trott]

The close event can fire twice if close is called twice. Move checks
to exit event for process instead.

Ref: #4476

[Trott]

Stress CI without the fix: https://ci.nodejs.org/job/node-stress-single-test/334/nodes=win-vs2015/console

Stress CI with this fix: https://ci.nodejs.org/job/node-stress-single-test/337/nodes=win-vs2015/console

[jbergstroem]

LGTM

[mscdex]

Couldn't this be changed to common.mustCall(fn, 2) instead?

[Trott]

Not as the test was written. Most of the times, it would run once, but every once in a while on Windows (and possibly elsewhere, but definitely on Windows), it would run twice.

In a stress test run, the test ran 9999 times on Windows. 9988 times, the function ran once. But 11 times, the test failed because it ran twice.

[cjihrig]

Changes LGTM, but shouldn't we be able to reliably determine how many times the close event occurs?

[Trott]

@cjihrig My original fix for this was to put in a boolean that indicated whether or not server.close() had been called yet and to check that boolean right before calling server.close(). This way, we could avoid the infrequent (but still occurring from time to time) situation where server.close() gets called by two different invocations of testConnect().

I refactored it to process.on('exit',...) because the resulting code is simpler this way and the change set is more straightforward as well.

The race condition exists because the client connection callback calls testConnect() (where server.close() happens) asynchronously (in the callback to client.close()) while the server calls it synchronously in its connection callback. If, at the end of the test, the client close callback manages to invoke testConnect() after the server has done so but before the program actually exits, then the close callback will fire twice. At least, that was my theory which appears to be borne out by the stress test results above.

[cjihrig]

@Trott would you mind taking a look at #4650. IMO, this test is overly complicated. It's calling testConnect() from everywhere, including multiple server events. I tried to simplify it in #4650.

[Trott]

@cjihrig Took a look. It looked good, but when I tried to confirm that it still triggered an assertion in Node 3.0.0 (by converting ES6 stuff to ES5), the test passed instead. So it no longer tests for the bug it was written to find.

Then it occurred to me that I didn't do a similar thing for this version of the test in this PR. So I've done that now and I can report that it still triggers the error:

$ node test/parallel/test-net-socket-local-address.js 

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: client and server should agree on the ports used
    at process.<anonymous> (/Users/trott/io.js/test/parallel/test-net-socket-local-address.js:43:10)
    at emitOne (events.js:77:13)
    at process.emit (events.js:169:7)

So that's good. For the record, here's the version of the test in this PR with unsupported-in-3.0.0 features rewritten or commented out:

'use strict';
// const common = require('../common');
const assert = require('assert');
const net = require('net');

// skip test in FreeBSD jails
// if (common.inFreeBSDJail) {
//   console.log('1..0 # Skipped: In a FreeBSD jail');
//   return;
// }

var conns = 0;
var clientLocalPorts = [];
var serverRemotePorts = [];

const server = net.createServer(function(socket) {
  serverRemotePorts.push(socket.remotePort);
  testConnect();
});

const client = new net.Socket();

server.listen(12346, '127.0.0.1', testConnect);

function testConnect() {
  if (conns > serverRemotePorts.length || conns > clientLocalPorts.length) {
    // We're waiting for a callback to fire.
    return;
  }

  if (conns === 2) {
    return server.close();
  }
  client.connect(12346, '127.0.0.1', function() {
    clientLocalPorts.push(this.localPort);
    this.once('close', testConnect);
    this.destroy();
  });
  conns++;
}

process.on('exit', function() {
  assert.deepEqual(clientLocalPorts, serverRemotePorts,
                   'client and server should agree on the ports used');
  assert.equal(2, conns);
});

If you can come up with a simpler test in that other PR that still finds the bug the test was written for, I'm happy to go with that instead of this PR, of course.

[cjihrig]

Closing in favor of #4650.