Skip to content
This repository was archived by the owner on Feb 1, 2022. It is now read-only.

Conversation

@yilmazdurmaz
Copy link
Contributor

In windows, assigning a port may take a little longer than previously assumed value of 2000ms.

I am using Windows 10 1903 64bit, and it takes about 3000ms to initialize a port.

I increased the timeout to some higher value (9999ms) to give a little more space to Windows to initialize a port to use with the app.

This patch probably solves issues #48, #54 and #70

In windows, assigning a port may take a little longer than previously assumed value of 2000ms.
I am using Windows 10 1903 64bit, and it takes about 3000ms to initialize a port.
I increased the timeout to some higher value (9999ms) to give a little more space to Windows to initialize a port to use with the app.

This patch probably solves issues nodejs#48, nodejs#54 and nodejs#70
@hybrist
Copy link
Collaborator

hybrist commented Aug 12, 2019

Hi - thanks for working on this! I'll try to review this eventually but unfortunately the bigger fire is that CI is currently broken for this repo (#72) and I don't have a lot of free time to work on this tool these days. :
(

@yilmazdurmaz
Copy link
Contributor Author

Cool commit! A few minor suggestions:

Why 9.999 seconds? If it takes 3000 seconds to get a port, why not, say, 5000 or 6000ms?
I'd add a comment just above the line explaining the reason for the long timeout so people don't have to read this commit message

// Windows can be slow to get a port - Windows 10 1903 64bit takes about 3000ms in testing

Better yet, actually check if the platform is windows and only increase the timeout then.

I tried different values and the code gets the port earlier if it is available, so adding a check for OS is not necessary. I agree commenting inside the code could be useful, I couldn't decide if I should. 9999ms is seemed logical as all other numbers would be random values just like the originally assumed 2000ms.
A little downside of the code is that the Timer is still kept set by the system even if port check completed less than a second. But it is a small price to pay I think.
Thanks

@yilmazdurmaz
Copy link
Contributor Author

Hi - thanks for working on this! I'll try to review this eventually but unfortunately the bigger fire is that CI is currently broken for this repo (#72) and I don't have a lot of free time to work on this tool these days. :
(

Hope you or someone with enough knowledge finds some time to work on it. For now, at least those that can access code can change the value just like I did.

I am new to this area of coding, maybe in few months I can help on that too.

@yilmazdurmaz
Copy link
Contributor Author

@jkrems
I dig into the testing parts a bit more. First with Travis CI system, then cloning the project to my local disk then running tests with few different methods.

  • There are many fails occurs with tap test suit. Error on CI possibly all are these fails. problems are mostly related to cli.output. it is somehow scrambled during execution.
  • I tried all test scripts by hand with node test/cli/test_file.js. only launch.test.js fails on tests at different steps on different runs. Each fail occurs on forwards child output tests, again with scrambled cli.output. They sometimes pass, sometimes fail, but at least 1 of them fails to the end.

I wrote my findings here in this thread, because I don't know what else to do with them. At least you/we may use it as a reference later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants