Skip to content

Loading…

Network utility for port sniffing needed by geddy #3

Merged
merged 3 commits into from

2 participants

@MiguelMadero
Collaborator

This relates to geddy's issue geddy/geddy#206

@larzconwell
Collaborator

Did you test this? I can get it to work correctly once, but after that it always gives the same arguments to the callback. Also I don't particularly like arguments given to the callback, we should ideally return an error(as errors can occur) and the boolean value. Finally we can't use that code exactly because it's the same as the other code I mentioned in the issue in Geddy, so if we used it we would have to include the license and everything.

@MiguelMadero
Collaborator

Uuups!!! Thanks for the feedback. I'll have a look now and update this.

@larzconwell
Collaborator

Nice man! That was faster than I was expecting haha. Thanks!

@larzconwell larzconwell merged commit 78c928b into mde:master
@MiguelMadero
Collaborator

Did I test it?
I only tested the simple end to end use case from geddy. I added some tests. I only cover the basics there. I will appreciate some feedback since is my first time writing tests in node. I'm not sure how you feel about having integration tests and if we should do something to distinguish them form true unit tests.
Also I don't like that I just randomly choose some ports for the tests and these ports might be in use in someone else's computer :( Not sure what's the best thing to do here.

I copied the code from somewhere else
I rewrote it from scratch. Same basic concept, it tries to connect to the given port, but different implementation.

Subsequent calls return the same arguments
While testing it I realized the callback was being called twice (onTimeout and when the socket gets closed on process exit). The new code doesn't have this problem. This is probably the error you were seeing.

Arguments in the callback.
I agree. The new code's callback takes an err object and the flag.

Also, what do you think about the name? I think using network and net can lead to some confusion.

Thanks again for the feedback. I apologize specially for the copy/pasted code.

@MiguelMadero
Collaborator

Wow that was fast. It took me longer to write a summary than for you to merge it :)
Anyway, let me know if you have any feedback.

Thanks

@MiguelMadero
Collaborator

BTW I also updated geddy/geddy#218

@larzconwell
Collaborator

I thought it was nice you added tests, we usually just write asserts so this is perfect haha.

I'm not sure about the name, I was thinking and the name is a bit odd to use because the name is hard to understand. An open port is a busy port, which is odd to think like that(for me anyway). But for now I think it's fine.

Later I may make something like, but I'm not sure yet.

utilities.network.isPortInUse(port, host callback) // checks if a port is open
utilities.network.isPortNotInUse(port, host, callback) // checks if a port is closed
utilities.network.checkPortStatus(status, port, host, callback) // checks a port for the given status
utilities.network.getPortStatus(port, host, callback) // gets the status of a given port
@MiguelMadero
Collaborator

I like isPortInUse. I was also thinking

utilities.network.canConnect(port, host, callback)

Feels like isPortInUse or even isPortOpen sounds fine for this particular use case, canConnect reflects better what the function is actually doing. Even if the port is open at the other end (think a different host), if we can't connect we'll still get false (or an err).

BTW, what do you mean by just writing asserts? I saw a few tests for the other utilities.

@larzconwell
Collaborator

Yeah that makes sense. Yeah pretty much all of our tests are just simply asserts for the different features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 9, 2012
  1. @MiguelMadero
Commits on Oct 10, 2012
  1. @MiguelMadero

    Added tests for network

    MiguelMadero committed
  2. @MiguelMadero

    Rewrote

    MiguelMadero committed
Showing with 90 additions and 0 deletions.
  1. +2 −0 lib/index.js
  2. +47 −0 lib/network.js
  3. +41 −0 test/network.js
View
2 lib/index.js
@@ -29,6 +29,7 @@ var utils = {}
, date = require('./date')
, request = require('./request')
, log = require('./log')
+ , network = require('./network')
// Third-party -- remove this if possible
, inflection = require('./inflection')
// Constructors
@@ -48,6 +49,7 @@ utils.object = object;
utils.date = date;
utils.request = request;
utils.log = log;
+utils.network = network;
utils.inflection = inflection;
utils.SortedCollection = SortedCollection;
utils.EventBuffer = EventBuffer;
View
47 lib/network.js
@@ -0,0 +1,47 @@
+var network
+ , net = require('net');
+
+/**
+ @name network
+ @namespace network
+*/
+network = new (function () {
+ /**
+ @name network#isPortOpen
+ @public
+ @function
+ @description Checks if the given port in the given host is open
+ @param {Number} port number
+ @param {String} host
+ @param {Function} callback Callback function -- should be in the format of function(err, result) {}
+ */
+ this.isPortOpen = function (port, host, callback) {
+ var isOpen = false
+ , connection
+ , error;
+
+ connection = net.createConnection(port, host, function () {
+ isOpen = true;
+ connection.end();
+ });
+
+ connection.on('error', function (err) {
+ // We ignore 'ECONNREFUSED' as it simply indicates the port isn't open. Anything else is reported
+ if(err.code !== 'ECONNREFUSED') {
+ error = err;
+ }
+ // the socket emits 'close' after 'error'. No need to do anything here
+ })
+
+ connection.setTimeout(400, function () {
+ connection.end();
+ });
+
+ connection.on('close', function (had_error) {
+ callback(error, isOpen);
+ });
+ };
+
+})();
+
+module.exports = network;
View
41 test/network.js
@@ -0,0 +1,41 @@
+var assert = require('assert')
+ , sys = require('sys')
+ , net = require('net')
+ , network = require('../lib/network')
+ , tests;
+
+tests = {
+
+ 'test a port is open': function (next) {
+ var expected = false
+ , port = 49152;
+
+ network.isPortOpen(port, null, function (err, isOpen) {
+ assert.ifError(err);
+ assert.equal(expected, isOpen);
+
+ next();
+ });
+
+ }
+
+, 'test a port is closed': function (next) {
+
+ var expected = true
+ , port = 49153
+ , server = net.createServer();
+
+ server.listen(port, function () {
+ network.isPortOpen(port, null, function (err, isOpen) {
+ assert.ifError(err);
+ assert.equal(expected, isOpen);
+
+ server.close(function () {
+ next();
+ });
+ });
+ });
+ }
+}
+
+module.exports = tests;
Something went wrong with that request. Please try again.