search

expressjs / vhost

virtual domain hosting
MIT License
761 stars 87 forks source link

fix: fixed stack overflow issue on 1200+ vhosts #45

Closed micheleriva closed 1 year ago

micheleriva commented 3 years ago

We had some issues while adopting vhost on a large codebase including a large set of vhosts. It looks like the express.js' next() callback is saturating the stack as it acts like a recursive function. By wrapping the next() callback call into a process.nextTick, we're able to free the call stack before pushing a new function call to it.

We tested this PR against up to ~50000 vhosts and it definitely solved the issue.

Feel free to ask if you have any questions!

dougwilson commented 3 years ago

Awesome, thanks! I marked this as needs tests, and in the meantime took a bit of a look. It actually looks like this should actually be solved in whatever is processing the middleware (i.e. Epxress) as, assuming I am replicating this correctly, this would affect any middleware, not just this one.

micheleriva commented 3 years ago

Hi @dougwilson, Thank you for your quick feedback 🙂

I've created a Repl to better explaining how we can replicate the error and how the fix I'm proposing will solve it: https://replit.com/@micheleriva/vhost-error-replica you can fork or clone this Repl on your machine to test the code.

Steps for replicating the error:

  1. Run the index.js file as it is
  2. Going to http://localhost:3000, you will see a stack overflow error
  3. Go back to the index.js file. Comment line 2 and uncomment line 3
  4. Restart the server. Go to http://localhost:3000. The error disappeared

Known issues

Replit (obviously) has a limited memory size, so if you go up to 50.000 routes, you will saturate the Replit RAM in any case. On my local machine (16GB RAM), I've been able to test the fix against up to 1.000.000 routes (vs. a limit of ~1200 routes when the fix is not applied).

I hope this clarifies a bit the context of this fix 🙂

micheleriva commented 3 years ago

Hi @dougwilson, Do you have any news on this one? Is there something missing on my side?

Please let me know if there's something I can do to improve the PR.

dougwilson commented 3 years ago

Ok, so it should get fixed at the source by https://github.com/pillarjs/router/commit/7d7c7f6b50c55efcbef20b8278ebf78810df1221

dougwilson commented 1 year ago

Closing as this has been fixed in the routing system itself now.