-
Notifications
You must be signed in to change notification settings - Fork 47
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
Update portscanner.js #13
Conversation
Added Flipped the order of the check...sometime we don't care about an error if the status matches. In the case of looking for an port not in use we actually want the timeout to trip.
foundPort = true | ||
callback(null, port) | ||
} | ||
else if (error) { | ||
callback(error) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand the problem. If you are looking for a closed
port, you will only see an error.
It looks like closed
and error
are always together. So, this change looks like it would work as expected, but I think we should expose both. What do you think about this change?
if (statusOfPort === status) {
foundPort = true
callback(error, port)
}
else if (error) {
callback(error)
}
So, if the status matches, we pass along the error as well (which is null in the "open" case and populated in the "closed" case). That way, if you care about the error, you can inspect it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
On Tue, Feb 4, 2014 at 4:42 PM, Sean Massa [email protected] wrote:
In lib/portscanner.js:
foundPort = true callback(null, port) }
else if (error) {
callback(error)
}
I think I understand the problem. If you are looking for a closed port,
you will only see an error.It looks like closed and error are always together. So, this change looks
like it would work as expected, but I think we should expose both. What do
you think about this change?if (statusOfPort === status) {
foundPort = true
callback(error, port)}else if (error) {
callback(error)}So, if the status matches, we pass along the error as well (which is null
in the "open" case and populated in the "closed" case). That way, if you
care about the error, you can inspect it.Reply to this email directly or view it on GitHubhttps://github.com//pull/13/files#r9442623
.
Austin Fatheree
RIVVIR Consulting
http://www.rivvir.com
[email protected]
832-483-0741
twitter: @afat http://twitter.com/afat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent. Make that change and I'll accept this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
On Tue, Feb 4, 2014 at 4:45 PM, Sean Massa [email protected] wrote:
In lib/portscanner.js:
foundPort = true callback(null, port) }
else if (error) {
callback(error)
}
Excellent. Make that change and I'll accept this.
Reply to this email directly or view it on GitHubhttps://github.com//pull/13/files#r9442727
.
Austin Fatheree
RIVVIR Consulting
http://www.rivvir.com
[email protected]
832-483-0741
twitter: @afat http://twitter.com/afat
passing along error
published in 0.2.1 |
In the code, a timeout while checking a port throws an error. This error gets included in the async call back and end the loop. But if your are checking for closed status this is a good thing and not an error...the port is good to go!
I flipped the order of the checks...sometime we don't care about an error if the status matches. In the case of looking for an port not in use we actually want the timeout to trip.
There may be a better way of doing this but for now it is making findAPortNotInUse work for me.