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: dns caching manager #1294

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

Conversation

fallenbagel
Copy link
Owner

Description

(wip)

Screenshot (if UI-related)

To-Dos

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

Issues Fixed or Closed

  • Fixes #XXXX

This is done as tmdb ttl is very less like 40 seconds so to make sure
any issues wont be caused due to cached dns (previously we were caching
for 5 minutes no matter what ttl)
@fallenbagel fallenbagel marked this pull request as draft January 20, 2025 13:49
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be removed. It was for testing quickly

Comment on lines +90 to +140
// Add DNS caching
dns.lookup = (() => {
const wrappedLookup = function (
hostname: string,
options:
| number
| dns.LookupOneOptions
| dns.LookupOptions
| dns.LookupAllOptions,
callback: (
err: NodeJS.ErrnoException | null,
address: string | dns.LookupAddress[] | undefined,
family?: number
) => void
): void {
if (typeof options === 'function') {
callback = options;
options = {};
}

dnsCache
.lookup(hostname)
.then((result) => {
if ((options as dns.LookupOptions).all) {
callback(null, [
{ address: result.address, family: result.family },
]);
} else {
callback(null, result.address, result.family);
}
})
.catch((error) => {
callback(error, undefined, undefined);
});
} as typeof dns.lookup;

(wrappedLookup as any).__promisify__ = async function (
hostname: string,
options?: dns.LookupAllOptions | dns.LookupOneOptions
): Promise<dns.LookupAddress[] | { address: string; family: number }> {
const result = await dnsCache.lookup(hostname);

if (options && 'all' in options && options.all === true) {
return [{ address: result.address, family: result.family }];
}
return { address: result.address, family: result.family };
};

return wrappedLookup;
})();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't you implement this inside the dnsCacheManagerItself? It floods the server a bit.

Copy link
Owner Author

@fallenbagel fallenbagel Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flood is coming from the debug logging right? Thats to be removed. I have it added rn to sort of test it. Or if not that what do you mean?

Copy link
Collaborator

@gauthier-th gauthier-th Jan 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No not that, i meant you could create a function inside server/utils/dnsCacheManager.ts that contains all this, because all this code to overwrite the lookup function takes a lot of space inside the startup function of the server.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. And then call it here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

const cached = this.cache.get(hostname);
if (cached) {
const age = Date.now() - cached.timestamp;
const ttlRemaining = Math.max(0, cached.ttl - age);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of the Math.max? Your code would work the same without it

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some issues where ttl went to negative values. That's why

server/utils/dnsCacheManager.ts Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants