-
Notifications
You must be signed in to change notification settings - Fork 505
Fix for issue #4960 PM2 cluster mode ECONNREFUSED errors #4961
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
base: main
Are you sure you want to change the base?
Conversation
Node.js 17+ changed DNS resolution to prefer IPv6 over IPv4. When running
in PM2 cluster mode, this causes "ECONNREFUSED ::1:8080" errors if the
backend only listens on IPv4 (127.0.0.1).
This fix sets dns.setDefaultResultOrder('ipv4first') at the very start
of the SSR bootstrap to ensure localhost resolves to IPv4.
See: nodejs/node#40537
🤖 Generated with [Claude Code](https://claude.ai/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good fix!
Would it be better to make the DNS resolution order configurable through config.yml (e.g. something like defaultDnsResultOrder: ipv4first)?
That way, we could easily change the preference between IPv4 and IPv6 without needing to update the code.
|
@jesielviana Thanks for the suggestion! I'm curious: are there scenarios where you'd want to configure this differently? I'm trying to think through when ipv6first (or other values) would be beneficial. My (limited!!!) understanding is that most backend deployments use dual-stack (both IPv4 and IPv6), so ipv4first should work regardless of whether the backend supports IPv6. Even if the backend has IPv6 support, trying IPv4 first just means it falls back to IPv6 if IPv4 fails. The issue we're fixing is specifically that Node.js 17+ changed to prefer IPv6, which causes connection failures in PM2 cluster mode when the Node process tries to connect via ::1 instead of 127.0.0.1. This happens even when the backend supports both protocols. That said, I might be missing something! If there's a deployment scenario where configurable DNS resolution order would be helpful, I'd be happy to add that. Do you have a specific use case in mind? |
- Change from namespace import to named import (import/no-namespace) - Move DNS configuration after all imports (import/first) - Fix import ordering (simple-import-sort/imports) All linting errors resolved. No functional changes to the DNS fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@bram-atmire I don’t have much experience in this area either, but I was thinking that in a dual-stack setup, some users might want to prioritize IPv6 over IPv4, depending on their infrastructure or network preferences. That said, I agree that ipv4first is a sensible default since it should work reliably in most environments. |
tdonohue
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bram-atmire : Thanks for this small PR. While I haven't tested it myself it seems logical to me. However, I do have a small suggestion regarding the inline comments below...I think we need to remove the header you've added to this file. Beyond that, it looks good. (Regarding the discussion above, I don't think we need to make this configurable until there's a good reason to do so. That can always be a future enhancement if there's not a good reason at this time.)
src/main.server.ts
Outdated
| * Fix for Node.js 17+ where DNS resolution prefers IPv6 over IPv4. | ||
| * This causes "ECONNREFUSED ::1:8080" errors in PM2 cluster mode when | ||
| * the backend only listens on IPv4. | ||
| * See: https://github.com/nodejs/node/issues/40537 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bram-atmire : Can we remove this comment from the header of the file, as this comment doesn't apply to the entire file. Instead, you can move some of these details down next to the new setDefaultResultOlder() call.. maybe say something like this:
// Apply DNS resolution order fix for Node.js 17+ by preferring IPv4 over IPv6.
// This fixes "ECONNREFUSED ::1:8080" errors in PM2 cluster mode the backend only listens on IPv4
// See https://github.com/DSpace/dspace-angular/issues/4960
setDefaultResultOrder('ipv4first');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdonohue done!
Address PR feedback: remove file header comment and place the explanatory comment directly next to the code it documents. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
References
Description
Node.js 17+ changed DNS resolution to prefer IPv6 over IPv4. When running in PM2 cluster mode, this causes "ECONNREFUSED ::1:8080" errors if the backend only listens on IPv4 (127.0.0.1).
This fix sets dns.setDefaultResultOrder('ipv4first') at the very start of the SSR bootstrap to ensure localhost resolves to IPv4.
See: nodejs/node#40537
Instructions for Reviewers
List of changes in this PR:
dns.setDefaultResultOrder('ipv4first')at the top ofsrc/main.server.tsbefore any other importsHow to test:
npm run build:prodpm2 start dspace-ui.json(ensureexec_mode: "cluster"in config)Note: This issue only occurs when:
Checklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation. (N/A - uses built-in Node.jsdnsmodule)