mojolicious / server-starter

:dizzy: Superdaemon
https://www.npmjs.com/package/@mojolicious/server-starter
Artistic License 2.0
8 stars 3 forks source link

Add EXPERIMENTAL support on Windows #1

Open dmanto opened 3 years ago

dmanto commented 3 years ago

So far the module only supports UNIX, as documented.

But the point is that not been able to support Windows can block a lot of possible contributions for open source projects, as it is estimated that half of node users are using it on Windows (win32 platform).

After trying to run mojo daemons in Windows, it is clear that neither fd=3 nor ?reuse=1 options seem to work on win32 (32 or 64 bits) systems.

However, opening the same port as the one selected by the superdaemon, seems to effectivelly allow the customer to open this connection, and with that all the tests just work.

As discussed in IRC #mojo chat, there may be up to 2 race situations in this case. That is because it seems reasonable that the superdaemon would first close the binded socket with some kind of error (because other process ask for an exclusive listen, and we see that succeds), and then the operating system happens to allow the client daemon to start listening in the same port.

However, this does not seem to be the case, as using this approach happen to work all the time (Tried MANY times to make it fail).

You can check that the server actually stops listening after the daemon client is launch, adding a couple console.logs inside the tap test, like:

  ...

  const server = await starter.newServer();
  const url = server.url();

  console.log('# node server listening before launch: ', server._srv.listening);
  await server.launch('perl', ['chat.pl', 'daemon', '-m', 'production', '-l', 'http://*?fd=3']);
  console.log('# node server listening after launch: ', server._srv.listening);

  const browser = await chromium.launch();
  const context = await browser.newContext();
  const page = await context.newPage();

  ...

And see that the superdaemon "fake" server is listening before the launch and stops listening after.

Anyway the proposed solution would not affect linux nor macOs platforms, and can be upgraded transparently if a better or more definitive solution is found for win32 users.

The proposed solution is to add a 'listenLocation' attribute that would be the one to use for the launch of the client daemon:

So in the listen() function inside server-starter.js program, add 'listenLocation' like:

    ...

    this.listenLocation = process.platform === "win32" ? this.url() : "http://*?fd=3";
    resolve();
});
...

That listenLocation can be passed as an argument in the case of "wrapped" tests, like:

await server.launch('perl', ['t/wrappers/change_title.pl', server.listenLocation]);

So it should be possible to read it in change_title.pl script as a command argument.

There are other minor changes needed (as the #!/usr/bin/env shebang will not work, but #!node is correctly interpreted by prove, and also some changes have to be made in order to make the javascript example in the README work on Linux / Mac & win32 platforms.

I could prepare a PR if you wish, or I can help running some tests on windows if you don't have access to one.

kraih commented 3 years ago

Those race conditions exist, that's not a question. You just need to run many tests in parallel to trigger it. We've used that same approach in the Mojolicious tests a few years ago thinking the risk was low, but cpantesters results showed us that the assumption was wrong.

kraih commented 3 years ago

Out of curiosity i did try the approach on GitHub Actions and straight on my first attempt ran into the ECONNREFUSED race condition, where the server opens the listen socket too slow for the first fetch request. https://github.com/mojolicious/server-starter/runs/2210719320

dmanto commented 3 years ago

Just some feedback after some attemps:

Looking at "windows" branch, where the race condition for ECONNREFUSED happens almost always, started to investigate how much delay should be added to just pass the tests, at least in the windows PC i have access.

Adding more than 60 mS delay (I would suggest 100, or even 250 mS) at the end of launch function, the tests pass all the time.


...
    if (process.platform !== 'win32') return Promise.resolve;
    return new Promise(resolve => setTimeout(resolve, 100));
...

This delays are to compensate the fact that the server can actually be very lazy to start listening, and the spawn method called at superdaemon seems to be resolved almost inmediatelly. I never needed this delay when launching a mojo daemon, at least in my tests (I don't have an explanation for that).

Other approach could be to resolve launch after some message is received from the server at stdout / stderr streams. For instance, mojo daemon server normally send a "Listening at..." line to stderr when start listening out of the box, so we could use the stderr event to analyse the line and resolve launch promise accordingly.

Just thinking in a loud voice anyway, I know this wouldn't be pretty (but in the other hand it would resolve as soon as the server be listening).

kraih commented 3 years ago

And then you are testing an application that sets up database migrations and fixtures for a few seconds. And then you're dealing with Ci environments where performance changes from run to run and you're dealing with delays from 1 to 20 seconds. And so on... That approach will go nowhere.

kraih commented 3 years ago

We will also not make this module Mojolicious specific.

kraih commented 3 years ago

The only approach i can imagine that would work somewhat reliable (outside of race conditions where you end up losing the port to another process) is to have complex logic to actually probe the TCP port and wait for it to start accepting connections.