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

app.listen() error behavior is different in Express 4 vs. 5, if port is occupied #6191

Open
heidemn-faro opened this issue Nov 28, 2024 · 3 comments · May be fixed by expressjs/expressjs.com#1705
Labels

Comments

@heidemn-faro
Copy link

heidemn-faro commented Nov 28, 2024

Hi there,
Great to see that Express 5 with async request handlers is finally available :-)

I have a question about behavior that differs in Express 4 vs. 5, if the listen port is already in use.
I wanted to know if that's on purpose, or if it's a bug.
If it's on purpose, it should probably be mentioned here? https://expressjs.com/en/guide/migrating-5.html
Thanks for having a look!

// minimal-express.mjs

import express from 'express';
import expressPkg from 'express/package.json' with { type: "json" };

const app = express();
app.use(express.json());
app.get('/api/health', (req, res) => {
	res.status(200).json({ status: 'ok' });
});

console.log('Express ', expressPkg.version);
console.log('Starting Express server on port 7072...');
const server = app.listen(7072, '0.0.0.0', () => {
	console.log('Callback');
	if (!server.address()) {
		// This line is only reachable in Express 5, but not in Express 4.
		throw new Error('Failed to start server (Express 5).');
	}
	console.log('Listening on http://localhost:7072/ - e.g. http://localhost:7072/api/health');
});

Enviroment: WSL2, Ubuntu 22.04, Node 20 or 22

PRETTY_NAME="Ubuntu 22.04.5 LTS"
Linux MYHOSTNAME 5.15.167.4-microsoft-standard-WSL2 #1 SMP Tue Nov 5 00:21:55 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Node.js v20.18.1
Node.js v22.11.0 (same result)

Results with Express 4, if port is already in use:

Express  4.21.1
Starting Express server on port 7072...
node:events:497
      throw er; // Unhandled 'error' event
      ^

Error: listen EADDRINUSE: address already in use 0.0.0.0:7072
    at Server.setupListenHandle [as _listen2] (node:net:1904:16)
    at listenInCluster (node:net:1961:12)
    at doListen (node:net:2135:7)
    at process.processTicksAndRejections (node:internal/process/task_queues:83:21)
Emitted 'error' event on Server instance at:
    at emitErrorNT (node:net:1940:8)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  code: 'EADDRINUSE',
  errno: -98,
  syscall: 'listen',
  address: '0.0.0.0',
  port: 7072
}

Results with Express 5, if port is already in use:

Express  5.0.1
Starting Express server on port 7072...
Callback
file:///some/folder/minimal-express.mjs:14
                throw new Error('Failed to start server (Express 5).');
                ^

Error: Failed to start server (Express 5).
    at Server.<anonymous> (file:///some/folder/minimal-express.mjs:14:9)
    at Server.f (/some/folder/node_modules/once/once.js:25:25)
    at Object.onceWrapper (node:events:634:26)
    at Server.emit (node:events:519:28)
    at emitErrorNT (node:net:1940:8)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
@heidemn-faro heidemn-faro changed the title app.listen() working differently in Express 4 vs. 5, if port is occupied app.listen() error behavior is different in Express 4 vs. 5, if port is occupied Nov 28, 2024
@dpopp07
Copy link

dpopp07 commented Dec 3, 2024

Although I don't know for sure, the code indicates that this is on purpose. Comparing the implementation of the listen method in 4.x ...

express/lib/application.js

Lines 633 to 636 in 59fc270

app.listen = function listen() {
var server = http.createServer(this);
return server.listen.apply(server, arguments);
};

... with 5.x ...

express/lib/application.js

Lines 609 to 617 in 344b022

app.listen = function listen () {
var server = http.createServer(this)
var args = Array.prototype.slice.call(arguments)
if (typeof args[args.length - 1] === 'function') {
var done = args[args.length - 1] = once(args[args.length - 1])
server.once('error', done)
}
return server.listen.apply(server, args)
}

... the code now explicitly invokes the user-provided callback (if it exists) in the event of an error. I'm guessing this is to provide the user with more control over their errors, instead of being subject to a crashing app. However, I agree with you that this transfer of error-handling responsibility to the user in the listen method would be good to capture in the migration docs. I can open a PR for that soon.

dpopp07 added a commit to dpopp07/expressjs.com that referenced this issue Dec 3, 2024
Adds a note to the `migrating-5` guide alerting users that error events
received by a server will now cause the callback argument in `app.listen` to
be invoked. In Express 4, this was not the case and errors would be thrown.

Resolves expressjs/express#6191

Signed-off-by: Dustin Popp <[email protected]>
@heidemn-faro
Copy link
Author

Okay probably the Express user is now required to check the 1st argument of the callback.

const server = app.listen(7072, '0.0.0.0', (error) => {
	if (error) {
		// This line is only reachable in Express 5, but not in Express 4.
		console.error("Failed to start server (Express 5)");
		throw error;
	}
	console.log('Listening on http://localhost:7072/ - e.g. http://localhost:7072/api/health');
});

Works fine:

$ node /tmp/express-test/minimal-express.mjs
Express  5.0.1
Starting Express server on port 7072...
Failed to start server (Express 5)
file:///tmp/express-test/minimal-express.mjs:16
                throw error;
                ^

Error: listen EADDRINUSE: address already in use 0.0.0.0:7072
    at Server.setupListenHandle [as _listen2] (node:net:1817:16)
    at listenInCluster (node:net:1865:12)
    at doListen (node:net:2014:7)
    at process.processTicksAndRejections (node:internal/process/task_queues:83:21) {
  code: 'EADDRINUSE',
  errno: -98,
  syscall: 'listen',
  address: '0.0.0.0',
  port: 7072
}

dpopp07 added a commit to dpopp07/expressjs.com that referenced this issue Dec 4, 2024
Adds a note to the `migrating-5` guide alerting users that error events
received by a server will now cause the callback argument in `app.listen` to
be invoked. In Express 4, this was not the case and errors would be thrown.

Resolves expressjs/express#6191

Signed-off-by: Dustin Popp <[email protected]>
Co-authored-by: M. Heide <[email protected]>
dpopp07 added a commit to dpopp07/expressjs.com that referenced this issue Dec 4, 2024
Adds a note to the `migrating-5` guide alerting users that error events
received by a server will now cause the callback argument in `app.listen` to
be invoked. In Express 4, this was not the case and errors would be thrown.

Resolves expressjs/express#6191

Signed-off-by: Dustin Popp <[email protected]>
Co-authored-by: M. Heide <[email protected]>
@wesleytodd
Copy link
Member

wesleytodd commented Dec 4, 2024

I took a quick read here, but am a bit busy with other work so don't have too much time to add more here, just wanted to link the original PR for context: #3216

EDIT: and the original one with the more context on why this change was made. #2623

@bjohansebas bjohansebas added docs and removed question labels Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants