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

Express.js middlewares are reused when creating a second Express app #7031

Open
1cedsoda opened this issue Nov 10, 2023 · 5 comments
Open

Express.js middlewares are reused when creating a second Express app #7031

1cedsoda opened this issue Nov 10, 2023 · 5 comments
Labels
bug Something isn't working node.js Compatibility with Node.js APIs

Comments

@1cedsoda
Copy link

1cedsoda commented Nov 10, 2023

What version of Bun is running?

1.0.11+f7f6233ea

What platform is your computer?

Darwin 23.0.0 arm64 arm

What steps can reproduce the bug?

// bun version 1.0.11+f7f6233ea
// node version v18.18.2

import express from "express";

async function newServer() {
  const app = express();
  const x = Math.random();
  app.use((req, res, next) => {
    console.log("middleware x", x);
    res.send("Hello World");
  });
  return new Promise((resolve, reject) => {
    const listener = app.listen(9999, () => {
      resolve(listener);
    });
  });
}

async function main() {
  const listener = await newServer();
  await fetch("http://localhost:9999");
  await listener.close();

  const listener2 = await newServer();
  await fetch("http://localhost:9999");
  await listener2.close();
}

main();

Running bun x.js

middleware x 0.6306528523654233
middleware x 0.6306528523654233

Expected: numbers to be different
Got: numbers are same

Running node x.js

middleware x 0.63468082718277
middleware x 0.16123682051243216

Expected: numbers to be different
Got: numbers are different

What is the expected behavior?

Bun: numbers should be different (middlewares are redeclared)
Express: numbers should be different (middlewares are redeclared)

What do you see instead?

Bun: numbers are the same (middlewares are reused)
Express: numbers should be different (middlewares are redeclared

Findings

In Bun the express middlewares seem to be reused.

Additional information

node version v18.18.2
express version 4.18.2

@1cedsoda 1cedsoda added the bug Something isn't working label Nov 10, 2023
@1cedsoda 1cedsoda changed the title Express.js middlewares are reused when creating a new Express app Express.js middlewares are reused when creating a second Express app Nov 10, 2023
@Electroid Electroid added the node.js Compatibility with Node.js APIs label Nov 10, 2023
@Electroid
Copy link
Contributor

This is likely a difference in behaviour for when listening to the same port twice. We will fix it.

@1cedsoda
Copy link
Author

Can confirm, it only happens if the same port is used multiple times.

@liz3
Copy link
Contributor

liz3 commented Nov 14, 2023

I can confirm that calling server.stop(true) instead of server.stop() here: https://github.com/oven-sh/bun/blob/main/src/js/node/http.ts#L446
does fix the issue, @Electroid is this something which would be okay to do?

@Electroid
Copy link
Contributor

I can confirm that calling server.stop(true) instead of server.stop() here: https://github.com/oven-sh/bun/blob/main/src/js/node/http.ts#L446 does fix the issue, @Electroid is this something which would be okay to do?

Yep, that should be a good fix.

liz3 added a commit to liz3/bun that referenced this issue Nov 15, 2023
This forces the app close within the Bun.serve listener for node http,
if we don't do this, the issue appears where if you instantly recreate a listener on the same port,
its hitting the old server causing weird issues.

Fixes: oven-sh#7031
Related: oven-sh#6632
@1cedsoda
Copy link
Author

1cedsoda commented Apr 2, 2024

Still appears in 1.1.0+5903a6141

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working node.js Compatibility with Node.js APIs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants