[test-runner] Race condition when selecting an open port

[justinfagnani]

The use of portfinder to find an open port allows for a race condition when running multiple parallel instances of wtr.

The way portfinder (and most other open port finding libraries) works is that it attempts to open a given port by starting a server, and if that succeeds it closes the server and returns the port number to the caller. In the time between closing the server and the caller starting it's own server another process can then claim the port. This happens quite easily with port 8000 if two wtrs calls happen very quickly (as might happen with a parallel script runner like wireit :) ).

The best solution we found when building Web Component Tester and polyserve was to move the open port finding in-house by actually trying to start our server and retrying if we failed because of an EADDRINUSE. This way once we found an open port we didn't have to release it and try to claim it again right away.

[bennypowers]

So would you like the option to supply an async callback to determine the port, then advanced cases could do their own thing?

[justinfagnani]

I don't think that would eliminate the race condition - fundamentally there's no way in node to have a function that reliably returns an open port.

What you need is either for WTR to do its own open port finding by just trying to start the server, or to allow external code to actually start any servers. You could do this will a callback that's passed a startServer function:

export default config {
  startServer: async (({internalStartServer}) => {
    for (const port of SAUCE_PORTS) {
      try {
        const server = await internalStartServer({
            port,
        });
        console.log('WTR server running on port', port);
        return server;
      } catch (e) {
        // TODO: check if error is EADDRINUSE, if not rethrow
        console.error(e);
      }
    }
    throw new Error(`No available ports. Ports tried: ${JSON.stringify(ports)}`);
  },
};

[justinfagnani]

This is what I had to do to deterministically open the server on an open port (and choose from a set of ports taht work on Sauce, something I had forgotten was necessary!): https://github.com/lit/lit/pull/2837/files#diff-01dc16c7924341aa6779396f1b2c6de747c0d8fd9815be012a50cd82bd4f623e

[justinfagnani]

Would you be open to a fix for this that removed the portfinder dependency?

Fixing this would make using WTR with Wireit a lot nicer.

[bennypowers]

what do you have in mind, @justinfagnani ?

[justinfagnani]

Something like the code in https://github.com/lit/lit/pull/2837/files#diff-01dc16c7924341aa6779396f1b2c6de747c0d8fd9815be012a50cd82bd4f623e where startTestRunner, or the underlying server it creates, is called in a loop and used when it succeeds. That commit is probably more complicated that the upstream solution would be, since it has to do everything via events.

[bennypowers]

Seems legit to me. I'd approve a PR that came with tests. @LarsDenBakker WDYT?

[justinfagnani]

The other thing I'd like to add is a config option for a list of ports to scan. Like I mentioned, I had forgotten that you need to use specific ports with Sauce, and that was hard information to find. But rather than hardcode that specific list I think taking a list of ports in the config and then maybe also exports a list of good Sauce ports would be nice.

[bennypowers]

also sounds good to me

[RobbyRabbitman]

this issues also applies to nx workspaces, where you can have parallel tasks - right now you can only run them sequential which drastically slows down ci nx affected -t test --parallel 1