node-portastic icon indicating copy to clipboard operation
node-portastic copied to clipboard

Find function does not stop testing ports

Open Nicklason opened this issue 5 years ago • 6 comments

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 https://github.com/alanhoff/node-portastic/blob/d27c7d4d5c2eee620aff5e5f7804c28c78848ff0/lib/portastic.js#L37 and / or https://github.com/alanhoff/node-portastic/blob/d27c7d4d5c2eee620aff5e5f7804c28c78848ff0/lib/portastic.js#L53

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 avatar Mar 20 '20 01:03 Nicklason

This is related to https://github.com/alanhoff/node-portastic/issues/9

Nicklason avatar Mar 20 '20 01:03 Nicklason

Coverage Status

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

coveralls avatar Mar 20 '20 01:03 coveralls

Coverage Status

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

coveralls avatar Mar 20 '20 01:03 coveralls

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

jancurn avatar Mar 24 '20 22:03 jancurn

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.

Nicklason avatar Mar 25 '20 04:03 Nicklason

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

metalwarrior665 avatar Jun 03 '20 20:06 metalwarrior665