Race condition in test-fs-read-stream-fd-leak.js

[john-yan]

https://github.com/nodejs/node/blob/master/test/parallel/test-fs-read-stream-fd-leak.js

The intermittent assertion failure could be fired as follows:
assert.js:89
throw new assert.AssertionError({
^
AssertionError: no leaked file descriptors using destroy() (got 5)
at null._onTimeout (/sandbox/johnyan/github-node/test/parallel/test-fs-read-stream-fd-leak.js:37:16)
at Timer.listOnTimeout (timers.js:89:15)

The race condition is spotted in the following code:

    if (++i === loopCount) {
      clearTimeout(this);
      setTimeout(function() {
        assert.equal(0, openCount, 'no leaked file descriptors using ' +
                     endFn + '() (got ' + openCount + ')');
        openCount = 0;
        callback && setTimeout(callback, 100);
      }, 100);
    }

In the failing case, the first setTimeout function is fired before all streams are properly closed. If reduce the setTimeout period (say from 100 to 1), the failure rate is increased. Depends on the running environment (slow/fast), the failure rate varies. Can anyone suggest a better solution to this?

[mhdawson]

I think we are going to need a timeout as its the only way to give a good message in the case of the failure case when we don't get the expected close (the other option is to let the test hang and have the harness time it out but that does not give us the opportunity to provide additional info). What I'd suggest is that we have a longer timeout before generating an error but that we check at a shorter interval for the success case so that the test runs quickly unless an failure occurs