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

http: memory leak if a server is not closed #48604

Closed
mcollina opened this issue Jun 29, 2023 · 9 comments · Fixed by #48611
Closed

http: memory leak if a server is not closed #48604

mcollina opened this issue Jun 29, 2023 · 9 comments · Fixed by #48611
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem.

Comments

@mcollina
Copy link
Member

Version

v18.16.1

Platform

Mac

Subsystem

http

What steps will reproduce the bug?

Run

const http = require('http')

async function main () {
  let i = 0
  while (true) {
    if (i % 100 === 0) {
      global.gc()
    }

    if (i % 100000 === 0) {
      console.log(process.memoryUsage())
    }

    http.createServer((req, res) => {})
    i++
  }
}

main()

How often does it reproduce? Is there a required condition?

All the time

What is the expected behavior? Why is that the expected behavior?

The memory should not grow uncontrolled

What do you see instead?

Memory growing and after many iterations a crash

Additional information

No response

@mcollina mcollina added confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. labels Jun 29, 2023
@lpinca
Copy link
Member

lpinca commented Jun 29, 2023

To be honest I don't think this is a bug. It's like opening sockets and never closing them.

@lpinca
Copy link
Member

lpinca commented Jun 29, 2023

Sorry, my bad, I did not notice that there is no server.listen() in the example, but anyway I think that the "leak" is expected with that sync loop.

@lpinca
Copy link
Member

lpinca commented Jun 29, 2023

I can reproduce even with an async loop so it's not that.

@lpinca
Copy link
Member

lpinca commented Jun 29, 2023

This seems to fix the issue

diff --git a/lib/_http_server.js b/lib/_http_server.js
index 0242e7a089..571ed15c6d 100644
--- a/lib/_http_server.js
+++ b/lib/_http_server.js
@@ -549,7 +549,7 @@ function Server(options, requestListener) {
   this.timeout = 0;
   this.maxHeadersCount = null;
   this.maxRequestsPerSocket = 0;
-  setupConnectionsTracking(this);
+  // setupConnectionsTracking(this);
   this[kUniqueHeaders] = parseUniqueHeadersOption(options.uniqueHeaders);
 }
 ObjectSetPrototypeOf(Server.prototype, net.Server.prototype);

cc: @ShogunPanda

@theanarkh
Copy link
Contributor

Maybe we should call setupConnectionsTracking on listen event ?

@lpinca
Copy link
Member

lpinca commented Jun 30, 2023

It makes sense to me, there is no connection to track if the server is not listening.

@ShogunPanda
Copy link
Contributor

That is fine, but it does not explain the leak. I'll try to dig into this in the next few days.

@theanarkh
Copy link
Contributor

theanarkh commented Jun 30, 2023

I think because the anonymous function hold the server object.

setInterval(checkConnections.bind(server), server.connectionsCheckingInterval).unref();

@ShogunPanda
Copy link
Contributor

Yes, I confirm that is the issue. I'll move that call in a listen even as suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants