wait for tcp port available (win32) #2

dmanto · 2021-03-31T13:03:39Z

Wait for server ready on windows platforms. This helps to allow a minimal windows platforms support. Related to issue #1

kraih · 2021-03-31T13:32:12Z

This module follows the normal Mojolicious rules. So tests and documentation are still required.

kraih · 2021-03-31T13:33:15Z

Why is this a Windows only feature? I don't see any Windows specific code. It is actually counter to the whole argument for adding the feature in the first place. We could offer it for Windows, macOS and Linux., as a slightly less safe alternative to passing the file descriptor directly to the managed process.

kraih · 2021-03-31T13:50:20Z

lib/server-starter.js

@@ -90,7 +92,23 @@ class Server extends EventEmitter {
    });

    // Should be resolved by the "spawn" event, but that is only supported in Node 15+
-    return Promise.resolve;
+    if (process.platform !== 'win32') return Promise.resolve;
+    return new Promise((resolve) => {


There is no reject handler. What's supposed to happen when it times out?

the caller will do exactly the same (trying to connect), and will fail because the port is not ready.

A promise does not resolve on its own.

lib/server-starter.js

kraih · 2021-03-31T13:52:52Z

lib/server-starter.js

+      (function loop () {
+        const connection = net.connect(port, resolve);
+        connection.on('error', err => {
+          if (err.code === 'ECONNREFUSED') {


And if it's something else?

should resolve the promise. But it actually doesn't, my mistake, see my answer about the logic a few minutes ago

It should reject the promise, not resolve.

The only thing the loop does is to wait enough time to get the server in a defined (either right or wrong) state. So if there is an error different from ECONNREFUSED there is no need to stay waiting, there is no argue on that.

But I think there is no added value in reject the promise with an error either. This is because you will try to connect by yourself after the launch of the server, and you will be sucesfull or not, and get the right error in any case.

Anyway the promise could be rejected when there is an error detected, the change itself is trivial (although the specific tests are not that easy) but I cannot think of any use case in which this can be beneficial for the user of the module.

kraih · 2021-03-31T13:53:25Z

lib/server-starter.js

+        const connection = net.connect(port, resolve);
+        connection.on('error', err => {
+          if (err.code === 'ECONNREFUSED') {
+            if (new Date() < timeToStop) {


And if it times out we just do nothing?

yes the caller will take care, I mean will recive the exact same error

How does the error reach the caller?

The logic is like:

wait up to win32Timeout mS, as long as the error you are getting is ECONNREFUSED.

if you could connect, resolve the promise

if you get other error, you will not retry

so Ooops, I need to resolve at the end of the anonymous function (or reject, but that will be handled much better by the caller, when it tries to connect)

kraih · 2021-03-31T14:01:43Z

lib/server-starter.js

@@ -64,12 +64,14 @@ class Server extends EventEmitter {
   * @param {object} [options] - Optional settings
   * @param {boolean} [options.stdout=false] - Forward server output from STDOUT to STDOUT
   * @param {boolean} [options.stderr=true] - Forward server output from STDERR to STDERR
+   * @param {number} [options.win32Timeout=30000] - (Windows only) Max time to wait for server ready, in mS


As far as i can see there is no reason at all for this to be Windows specific.

It will have no effect on other platforms, at least how is it written.

If you mean to wait for connection ready at the TCP port in all cases, it could be done.

The way it is done now makes little sense. Since the module doesn't become more portable, it just allows someone to use it in a Windows specific way. Instead of enabling one test to run on all platforms.

kraih · 2021-03-31T14:48:28Z

So, to sum up the requirements. a) The feature needs to work on all platforms, Windows, macOS and Linux. b) There needs to be an additional documentation section explaining how to use the module in a portable (but slightly less safe) way. c) There need to be new tests for success (random and specific port) and failure cases (timeout, failed web app start, connection error other than ECONNREFUSED).

…tion, as logic turned out to be very different

dmanto · 2021-04-06T15:12:55Z

I hope we still have a chance to get to a usable feature on this issue.
And as the other PR was a mistake, so we leave it closed, but I will address all issues mentioned by you in the following points.
Regarding your sum up on requirements above:
a) The feature should work in Linux, macOS and Windows: done, and tested on all of them,
b) Additional documentation: done, added a section on launching servers with this "portable" feature,
c) New tests: (all of them in a new script: test/start_port.js, which is a modified version of test/start_fd.js)
- success (random port): done,
- success (specific port): done,
- timeout, done (reduced to 500 mS so we don't slow down tests too much),
- failed web app start: done, we got a timeout plus server emitted specific error,
- connection error other than ECONNREFUSED: done, we got a timeout plus server emitted specific error
Also regarding to tests, I needed a way to skip fd tests on windows, so I used some tap functionality to skip them all on win32 platforms. It may look hacky, so just let me know if you know a cleaner way.
About the "no reject handler" remark: the reject case was added so is fixed now (done). Also I want to apologize for my misunderstanding on the process, I answered in the conversation with some explanation in the timeout logic and thought that was enough, that's why I included the comment in the code (In no way I meant to offend you nor to ignore any of your comments).
About the "this constant does nothing" remark, sorry I had missed your last comment, is fixed now (done).
About the "me still missing the point" as this is not meant to be a Mojolicious specific module, I think I understand that. I removed the ENV variable used to avoid fd passing, and also the "listenAddress()" function, so to avoid any "Hidden magic" as you mentioned on other remark.
To let the customer choose how to launch the child server process, a new launch function, launchPortable(), is defined.
The reason I separate both cases is that the logic changes a lot between them, as launchPortable needs the port to be closed before spawning the server, otherwise the "first" race condition will trigger a lot (almost always on a mac). launch() in the other hand, needs to close the port after the server opened the passed fd. And for the retry and timeout functionality, I cannot see any case in which that functionality would make sense when using fd passing, as the port should be up all the time.

kraih and others added 8 commits March 28, 2021 16:40

Link to blog post

ed21f8d

Use getters for pid and port

35cb5c8

Small documentation improvements

0d9dd39

Use a little less code

c7cf6c7

The index.js file is not needed

34bc5e6

Bump version

5c589fd

newServer returns a Promise

fbd8076

wait for tcp port available

5b75dcd

kraih requested changes Mar 31, 2021

View reviewed changes

kraih reviewed Mar 31, 2021

View reviewed changes

kraih added the work in progress label Mar 31, 2021

kraih and others added 12 commits March 31, 2021 16:51

Enable mergify

d1bf330

Make room for port based tests

5cf785d

Test port assignment

a58db73

Give the test a little more time to close the port

425fcde

Wait for the first listen socket to be closed

7865e33

add listenAddress attribute

cccc7ed

avoid fdpass through env var

9aef07f

fix lint errors

bc67f2d

add slow server tests

1f0f1af

fix lint again

3ef6fc5

modify README for non fd passing cases

3493b4b

add minimal windows support

e99fd6e

dmanto closed this Apr 4, 2021

dmanto reopened this Apr 5, 2021

dmanto added 2 commits April 5, 2021 14:49

small fixes

030cdbf

add avoidFdPassing option to launch funtion

8663e04

dmanto added 13 commits April 5, 2021 17:39

remove listenAddress() function, improve README

08d5c04

add race condition caveat

18c0dd2

fix typos and rephrase race condition caveat

8b1b9f7

fix typos

293263c

add windows workflow, but skiping start_fd.js tests in this case

47797fe

reorganize README

c850246

manual merge into former branch

b8d7ef2

fix workflow

2d204f4

leave server_fd.js as in master branch

b05e0d8

fix typo

9140764

add a launchPortable() function instead of checking avoidFdPassing op…

45f29f3

…tion, as logic turned out to be very different

fix README intralink

e526c69

more tests on portable mode

8ab9dd8

dmanto added 4 commits April 6, 2021 18:37

fix node 10.x windows cannot find module error

cf15c04

fix bad port test on windows node 10.x

575e5d7

fix bad port test, try harder

4fbb40b

skip some tests on node 10.x (windows), seems to have error msgs broken

054e412

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wait for tcp port available (win32) #2

wait for tcp port available (win32) #2

dmanto commented Mar 31, 2021

kraih commented Mar 31, 2021

kraih commented Mar 31, 2021 •

edited

Loading

kraih Mar 31, 2021

dmanto Mar 31, 2021

kraih Mar 31, 2021

kraih Mar 31, 2021

dmanto Mar 31, 2021

kraih Mar 31, 2021

dmanto Apr 5, 2021

kraih Mar 31, 2021

dmanto Mar 31, 2021

kraih Mar 31, 2021

dmanto Mar 31, 2021 •

edited

Loading

kraih Mar 31, 2021

dmanto Mar 31, 2021

kraih Mar 31, 2021

kraih commented Mar 31, 2021

dmanto commented Apr 6, 2021

wait for tcp port available (win32) #2

Are you sure you want to change the base?

wait for tcp port available (win32) #2

Conversation

dmanto commented Mar 31, 2021

kraih commented Mar 31, 2021

kraih commented Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmanto Mar 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kraih commented Mar 31, 2021

dmanto commented Apr 6, 2021

kraih commented Mar 31, 2021 •

edited

Loading

dmanto Mar 31, 2021 •

edited

Loading