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

Find function does not stop testing ports #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nicklason
Copy link

This is a problem I found while using https://www.npmjs.com/package/proxy-chain.

Explanation

The proxy-chain module has a function to create a proxy server using a random open port, it finds the port using portastic. One of the ports that was tested gave an error and my PC would freeze for a few seconds after making the proxy server.

image

I am not sure why my PC freezes, but it is a result of the rejection.

The rejection comes from portastic. It would reject on errors that would occure when opening a server. It rejects after the proxy server is made, so something was messing things up in the background while the server was working fine.

The rejection comes from

def.reject(err);
and / or
return def.reject(err);

The rejection is fine, it is just because it is not caught. But when using the find function all exceptions should be caught anyway. I don't change that in this pull request, because I don't think it is that big of a problem.

The biggest issue, and the reason for this pull request, is that the find function does not cancel when it finds the asked amount of open ports. Proxy-chain tells portastic to find a single open port in the range 20000 to 60000, but portastic fails to stop testing ports / cancel the promise.

I have replaced it with a recursive function, that way it is easier to control when the find function should stop, and actually make it stop.

Replicate

To replicate the problem use the following code:

process.env.DEBUG = 'portastic:*';

const portastic = require('portastic');

const opts = {
    min: 20000,
    max: 60000,
    retrieve: 1
};

console.log(opts);

portastic.find(opts).then(function (ports) {
    console.log(ports);
});

It will find the first open port and resolve the promise, but it will keep testing. See the image below for the debug log:
image

With my fix:
image

@Nicklason
Copy link
Author

This is related to #9

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 87.097% when pulling 473eec1 on Nicklason:master into d27c7d4 on alanhoff:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 87.097% when pulling 473eec1 on Nicklason:master into d27c7d4 on alanhoff:master.

@jancurn
Copy link

jancurn commented Mar 24, 2020

Great job, thanks for that! Any chance to merge this soon?

@Nicklason
Copy link
Author

It was last updated 4 years ago, I doubt it will be merged any time soon.

A different thing I noticed is that the node_modules folder is published in the module on npm. So when it does get merged, hopefully the folder won’t be published aswell.

@metalwarrior665
Copy link

Omg! This would be awesome! Didn't even know this PR existed, this annoying error is spamming my log from the dawn of time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants