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

Bug: DoH client is missing in-memory cache #59

Open
lidel opened this issue Feb 27, 2024 · 15 comments
Open

Bug: DoH client is missing in-memory cache #59

lidel opened this issue Feb 27, 2024 · 15 comments
Assignees
Labels
kind/stale need/author-input Needs input from the original author
Milestone

Comments

@lidel
Copy link
Member

lidel commented Feb 27, 2024

Found the problem can be seen when opening http://localhost:3000/ipns/docs.ipfs.tech without subdomain isolation (e.g. commit 1ef0094).

Bunch of times /ipfs/assets/fooo is being resolved (separate problem, but will go away when I finish #30):

image

This edge case surfaced a weakness in our DoH implementation: no local cache.
And each time we trigger the same DNS lookup for _dnslink.assets:

image

This works fast only because we added cache in https://github.com/ipshipyard/waterworks-infra/pull/30, but is wasteful and eats into the pool of HTTP request our SW can make.

We need to have in-memory cache which ideally respects TTL from DNS, or at least caches results for 1 minute before asking upstream for an update.

@lidel lidel added the bug Something isn't working label Feb 27, 2024
@SgtPooki
Copy link
Member

this should be resolved with the latest helia-verified-fetch and new dns updates. will confirm and then resolve

@2color
Copy link
Member

2color commented Mar 26, 2024

I just tried with inbrowser.dev and it seems that even with ipfs/helia-verified-fetch#19 merged and included in the latest release, caching is not working properly. For example see in the second screenshot the logs to the console, most times it isn't resolved from cache.

Screenshot 2024-03-26 at 1 02 20 pm
Screenshot 2024-03-26 at 1 06 24 pm

I'm gonna look into this.

@2color
Copy link
Member

2color commented Mar 26, 2024

ipfs/helia-verified-fetch#34 should resolve this.

I was under the false impression that we cache the DNSLink record in the datastore inside @helia/ipns, but that doesn't seem to be the case.

@2color 2color self-assigned this Mar 26, 2024
@2color
Copy link
Member

2color commented Mar 26, 2024

Strange that even though we had a bug (resolved by ipfs/helia-verified-fetch#34) with the cache in verified-fetch, it seems that @multiformats/dns used by @helia/ipns wasn't returning from its own cache, which results in many DoH requests.

I'll investigate this

@SgtPooki
Copy link
Member

SgtPooki commented Apr 4, 2024

we should be able to close this once the latest @helia/ipns is released

@SgtPooki
Copy link
Member

SgtPooki commented Apr 8, 2024

helia ipns is released. we need to update verified-fetch and then update here.

https://github.com/ipfs/helia/releases/tag/ipns-v7.2.0

@SgtPooki SgtPooki added this to the Alpha milestone Apr 8, 2024
@SgtPooki SgtPooki self-assigned this Apr 15, 2024
@SgtPooki
Copy link
Member

This issue is resolved on main.

@2color
Copy link
Member

2color commented Apr 16, 2024

Also confirming that from my tests this is resolved!

@SgtPooki SgtPooki reopened this May 7, 2024
@SgtPooki
Copy link
Member

SgtPooki commented May 7, 2024

@aschmahmann reported issues with multiple dns-queries still, but we're having trouble reproducting.

Adin, can you provide the commit hash (found in HTML of root domain) of the site you're using, and a screenshot of what you're seeing for "multiple dns queries" ?

@aschmahmann
Copy link
Contributor

aschmahmann commented May 8, 2024

@SgtPooki not able to see within the same browser request the HTML of the root domain to get the commit hash, but I've got this as the loaded js and there's a tag on the JS file name that IIUC should help with versioning.

version

and here are the multiple dns requests

dns-queries

@SgtPooki
Copy link
Member

SgtPooki commented May 8, 2024

@aschmahmann you should be able to go to <url>/#/ipfs-sw-config to get the config page, which still uses the same index.html, which will have the commit hash. That will make tracking things down a lot easier.

image

@SgtPooki
Copy link
Member

@aschmahmann do you still get this error on inbrowser.dev?

@SgtPooki
Copy link
Member

confirmed this is happening intermittently for Adin on latest. might be a race condition? might be CORS pre-flight being blocked causing issues? we need to dive in deeper, but it's a hard to repro problem that is inconsistent

@SgtPooki
Copy link
Member

I believe this was fixed by #435 & #436

@aschmahmann (or anyone) can you confirm this is still happening on inbrowser.dev ?

@SgtPooki SgtPooki added need/author-input Needs input from the original author and removed bug Something isn't working labels Nov 13, 2024
Copy link
Contributor

Oops, seems like we needed more information for this issue, please comment with more details or this issue will be closed in 7 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/stale need/author-input Needs input from the original author
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

4 participants