Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wait for tcp port available (win32) #2

Open
wants to merge 39 commits into
base: windows
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
ed21f8d
Link to blog post
kraih Mar 28, 2021
35cb5c8
Use getters for pid and port
kraih Mar 28, 2021
0d9dd39
Small documentation improvements
kraih Mar 29, 2021
c7cf6c7
Use a little less code
kraih Mar 29, 2021
34bc5e6
The index.js file is not needed
kraih Mar 29, 2021
5c589fd
Bump version
kraih Mar 29, 2021
fbd8076
newServer returns a Promise
kraih Mar 30, 2021
5b75dcd
wait for tcp port available
dmanto Mar 31, 2021
d1bf330
Enable mergify
kraih Mar 31, 2021
5cf785d
Make room for port based tests
kraih Mar 31, 2021
a58db73
Test port assignment
kraih Mar 31, 2021
425fcde
Give the test a little more time to close the port
kraih Apr 1, 2021
7865e33
Wait for the first listen socket to be closed
kraih Apr 1, 2021
cccc7ed
add listenAddress attribute
dmanto Apr 3, 2021
9aef07f
avoid fdpass through env var
dmanto Apr 4, 2021
bc67f2d
fix lint errors
dmanto Apr 4, 2021
1f0f1af
add slow server tests
dmanto Apr 4, 2021
3ef6fc5
fix lint again
dmanto Apr 4, 2021
3493b4b
modify README for non fd passing cases
dmanto Apr 4, 2021
e99fd6e
add minimal windows support
dmanto Apr 4, 2021
030cdbf
small fixes
dmanto Apr 5, 2021
8663e04
add avoidFdPassing option to launch funtion
dmanto Apr 5, 2021
08d5c04
remove listenAddress() function, improve README
dmanto Apr 5, 2021
18c0dd2
add race condition caveat
dmanto Apr 5, 2021
8b1b9f7
fix typos and rephrase race condition caveat
dmanto Apr 5, 2021
293263c
fix typos
dmanto Apr 5, 2021
47797fe
add windows workflow, but skiping start_fd.js tests in this case
dmanto Apr 5, 2021
c850246
reorganize README
dmanto Apr 5, 2021
b8d7ef2
manual merge into former branch
dmanto Apr 5, 2021
2d204f4
fix workflow
dmanto Apr 5, 2021
b05e0d8
leave server_fd.js as in master branch
dmanto Apr 5, 2021
9140764
fix typo
dmanto Apr 5, 2021
45f29f3
add a launchPortable() function instead of checking avoidFdPassing op…
dmanto Apr 6, 2021
e526c69
fix README intralink
dmanto Apr 6, 2021
8ab9dd8
more tests on portable mode
dmanto Apr 6, 2021
cf15c04
fix node 10.x windows cannot find module error
dmanto Apr 6, 2021
575e5d7
fix bad port test on windows node 10.x
dmanto Apr 6, 2021
4fbb40b
fix bad port test, try harder
dmanto Apr 6, 2021
054e412
skip some tests on node 10.x (windows), seems to have error msgs broken
dmanto Apr 7, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion lib/server-starter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @returns {Promise}
*/
launch (cmd, args, options = {}) {
if (typeof this._process !== 'undefined') throw new Error('Server already launched');
const stdout = typeof options.stdout !== 'undefined' ? options.stdout : false;
const stderr = typeof options.stderr !== 'undefined' ? options.stderr : true;
const win32Timeout = typeof options.win32Timeout !== 'undefined' ? options.win32Timeout : 30000;

const proc = (this._process = spawn(cmd, args, { stdio: ['pipe', 'pipe', 'pipe', this._fd] }));
this._srv.close();
Expand All @@ -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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A promise does not resolve on its own.

const retryTime = 60;
dmanto marked this conversation as resolved.
Show resolved Hide resolved
const now = new Date();
dmanto marked this conversation as resolved.
Show resolved Hide resolved
const timeToStop = new Date(now.getTime() + win32Timeout);
const port = this.port;
(function loop () {
const connection = net.connect(port, resolve);
connection.on('error', err => {
if (err.code === 'ECONNREFUSED') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if it's something else?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should reject the promise, not resolve.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (new Date() < timeToStop) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if it times out we just do nothing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the error reach the caller?

Copy link
Author

@dmanto dmanto Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

setTimeout(loop, retryTime);
}
}
});
})();
});
}

/**
Expand Down