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

Include listen errors in app.listen() callback #2623

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ var deprecate = require('depd')('express');
var merge = require('utils-merge');
var resolve = require('path').resolve;
var slice = Array.prototype.slice;
var eeFirst = require('ee-first');

/**
* Application prototype.
Expand Down Expand Up @@ -592,7 +593,14 @@ app.render = function(name, options, fn){

app.listen = function(){
var server = http.createServer(this);
return server.listen.apply(server, arguments);
var args = Array.prototype.slice.call(arguments);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, I found an even simpler/more robust solution to this: call .listen and just see if there is now a listening event listener attached, and use that :)

if (typeof args[args.length - 1] === 'function'){
var done = args.pop();
eeFirst([[server, 'error', 'listening']], function(err, ee, event, eventArgs){
done.apply(ee, eventArgs);
});
}
return server.listen.apply(server, args);
};

/**
Expand Down
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"cookie-signature": "1.0.6",
"debug": "~2.1.3",
"depd": "~1.0.0",
"ee-first": "~1.1.0",
"escape-html": "1.0.1",
"etag": "~1.5.1",
"finalhandler": "0.3.4",
Expand All @@ -49,8 +50,8 @@
"send": "0.12.2",
"serve-static": "~1.9.2",
"type-is": "~1.6.1",
"vary": "~1.0.0",
"utils-merge": "1.0.0"
"utils-merge": "1.0.0",
"vary": "~1.0.0"
},
"devDependencies": {
"after": "0.8.1",
Expand Down
15 changes: 15 additions & 0 deletions test/app.listen.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@

var express = require('../')
, assert = require('assert')
, request = require('supertest');

describe('app.listen()', function(){
Expand All @@ -15,4 +16,18 @@ describe('app.listen()', function(){
done();
});
})

it('should callback on HTTP server errors', function(done){
var app = express();
var app2 = express();

var server = app.listen(9999, function(err){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this port number be made dynamic? Getting some race conditions where the Express suite is running multiples times on the same machine and they all want to use port 9999. Just have app listen randomly and have app2 listen to whatever port app is listening on should work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can make this change for sure. Let me know if you also want me to move this PR to the 5.0 branch and I will do it all at once.

assert(err === undefined);
app2.listen(9999, function(err){
assert(err.code === 'EADDRINUSE');
server.close();
done();
});
});
})
})