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

feat(settings): add settings for custom DNS servers and IPv4 resolution first #1266

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

gauthier-th
Copy link
Collaborator

Description

This PR adds settings to change the DNS servers Jellyseerr uses and to force Jellyseerr to resolve DNS queries using IPv4 first. These settings aim to make it easier for less experienced users to fix network errors related to DNS resolution.

Screenshot (if UI-related)

image

To-Dos

  • Successful build pnpm build
  • Translation keys pnpm i18n:extract
  • Database migration (if required)

…on first

This PR adds settings to change the DNS servers Jellyseerr uses and to force Jellyseerr to resolve
DNS queries using IPv4 first. These settings aim to make it easier for less experienced users to fix
network errors related to DNS resolution.
@Shadowghost
Copy link

Out of intereste: Doesn't the used request framework support something like Happy Eyeballs for DNS resolution?

@gauthier-th
Copy link
Collaborator Author

gauthier-th commented Jan 16, 2025

Out of intereste: Doesn't the used request framework support something like Happy Eyeballs for DNS resolution?

Node.js has a "loosely implementation" of Happy Eyeballs, that is enabled by default.

It doesn't appear to solve issues on broken IPv6 stacks. In fact, we disable it with when forcing to IPv4 first (net.setDefaultAutoSelectFamily(false);).
From the tests we have done with users having this kind of issue, it seems that this solves their problem.

If you have any idea regarding this, I'd be glad to hear it, because we struggled quite a lot on this as we almost never manage to replicate the issue.

@gauthier-th gauthier-th merged commit 7fcc0eb into develop Jan 16, 2025
7 checks passed
@fallenbagel fallenbagel deleted the network-settings branch January 16, 2025 09:50
@fallenbagel
Copy link
Owner

🎉 This PR is included in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@onedr0p
Copy link
Contributor

onedr0p commented Jan 16, 2025

@gauthier-th @fallenbagel

I don't want to get in an argument but applications should not support changing the network stack (DNS) at this low of a level, this feature should be reverted IMO. You can take a look around at self-hosted apps and I am willing to bet the majority (if not all) of them do not support this feature nor should they.

@gauthier-th
Copy link
Collaborator Author

@gauthier-th @fallenbagel

I don't want to get in an argument but applications should not support changing the network stack (DNS) at this low of a level, this feature should be reverted IMO. You can take a look around at self-hosted apps and I am willing to bet the majority (if not all) of them do not support this feature nor should they.

We understand this shouldn't belong to the application land, but we had (and still have) issues everyday of users complaining because of the network error. This setting came as a convenient way for us and for users (especially for non-technical users) to fix the issue no matter how Jellyseerr has been installed.

We're using the Fetch API of the latest LTS version of Node.js, which is nowadays the "standard" way of doing requests in Node.js and JavaScript.
From our point of view, our choices came down to: reverting to the Axios (a third party networking library, with a different DNS and request mechanism) which we had previously migrated to Fetch, or adding this option to make our lives easier.

As I said earlier, we're open to suggestions as we're not network guys, so it's hard for us to figure out what can be done here.

@DatGuy1
Copy link

DatGuy1 commented Jan 17, 2025

This PR introduced a bug where dnsServers does not exist in settings.main, leading to TypeError: Cannot read properties of undefined (reading 'trim') at https://github.com/Fallenbagel/jellyseerr/blob/develop/server/index.ts#L83

@gauthier-th
Copy link
Collaborator Author

@DatGuy1 we can't reproduce your issue. It may be related to a misconfiguration on your side.
Can you please hop onto Discord so we can figure this out?

@fallenbagel
Copy link
Owner

This PR introduced a bug where dnsServers does not exist in settings.main, leading to TypeError: Cannot read properties of undefined (reading 'trim') at https://github.com/Fallenbagel/jellyseerr/blob/develop/server/index.ts#L83

If you're installing from source, you might need to delete .dist and .next folder before rebuilding.

@gauthier-th
Copy link
Collaborator Author

@gauthier-th @fallenbagel.
I don't want to get in an argument but applications should not support changing the network stack (DNS) at this low of a level, this feature should be reverted IMO. You can take a look around at self-hosted apps and I am willing to bet the majority (if not all) of them do not support this feature nor should they.

We understand this shouldn't belong to the application land, but we had (and still have) issues everyday of users complaining because of the network error. This setting came as a convenient way for us and for users (especially for non-technical users) to fix the issue no matter how Jellyseerr has been installed.

We're using the Fetch API of the latest LTS version of Node.js, which is nowadays the "standard" way of doing requests in Node.js and JavaScript. From our point of view, our choices came down to: reverting to the Axios (a third party networking library, with a different DNS and request mechanism) which we had previously migrated to Fetch, or adding this option to make our lives easier.

As I said earlier, we're open to suggestions as we're not network guys, so it's hard for us to figure out what can be done here.

I dug a bit about this, and came across this interesting issue: nodejs/node#54359

It says that:

this is a known technical limitation of Node and its implementation. The reason is that there is no way for us to cancel successful connection if another goes through so we chose to only have one attempt running at any time.

So Node.js doesn't properly implement the Happy Eyeballs mechanism, but does 2 requests, one at a time.
I'm confident enough to say that this is the source of many of our problems with request timeouts.

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 this pull request may close these issues.

5 participants