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

Duplicate block requests #104

Closed
lidel opened this issue Mar 12, 2024 · 10 comments · Fixed by ipfs/helia#503 or #212
Closed

Duplicate block requests #104

lidel opened this issue Mar 12, 2024 · 10 comments · Fixed by ipfs/helia#503 or #212
Assignees
Milestone

Comments

@lidel
Copy link
Member

lidel commented Mar 12, 2024

Opening https://vitalik-eth.ipns.inbrowser.link/ on empty cache and empty browser profile requests https://trustless-gateway.link/ipfs/QmNTP17BS7AEHn4TuiyKYtKq1StCMxxXRSt4gpiTdyuHRB?format=raw five times:

2024-03-12_17-28

To see it, remember to preserve log, as SW will

2024-03-12_17-30

@SgtPooki
Copy link
Member

SgtPooki commented Mar 12, 2024

could this be due to the prefetch we do when config is reloaded? https://github.com/ipfs-shipyard/helia-service-worker-gateway/blob/089cacded5d0e74dce6548fa8484b973bbf15ebd/src/redirectPage.tsx#L35

When we implement caching (#73), no more duplicate requests should be sent

@SgtPooki SgtPooki added this to the Alpha milestone Apr 8, 2024
@SgtPooki
Copy link
Member

FYI I am still seeing this today, but we don't have the cache updates in fully yet

image

@SgtPooki SgtPooki self-assigned this Apr 16, 2024
@SgtPooki
Copy link
Member

FYI I think ipfs/helia#483 & ipfs/helia-verified-fetch#50 resolves this.

@2color
Copy link
Member

2color commented Apr 18, 2024

I've been looking into this issue and wanted to point out that the CID in question: QmNTP17BS7AEHn4TuiyKYtKq1StCMxxXRSt4gpiTdyuHRB is the css unixfs directory in the root of Vitalik's website.

The problem is that the Service Worker intercepts 5 concurrent requests to load css (all under the css folder) files after loading the initial HTML:

helia:service-worker-gateway helia-sw: incoming request url: https://vitalik-eth.ipns.inbrowser.dev/css/common-vendor.b8ecfc406ac0b5f77a26.css: +8ms
sw.ts:151 helia:service-worker-gateway helia-sw: incoming request url: https://vitalik-eth.ipns.inbrowser.dev/css/fretboard.f32f2a8d5293869f0195.css: +1ms
sw.ts:151 helia:service-worker-gateway helia-sw: incoming request url: https://vitalik-eth.ipns.inbrowser.dev/css/pretty.0ae3265014f89d9850bf.css: +0ms
sw.ts:151 helia:service-worker-gateway helia-sw: incoming request url: https://vitalik-eth.ipns.inbrowser.dev/css/pretty-vendor.83ac49e057c3eac4fce3.css: +0ms
sw.ts:151 helia:service-worker-gateway helia-sw: incoming request url: https://vitalik-eth.ipns.inbrowser.dev/css/global.css: +1ms
sw.ts:151 helia:service-worker-gateway helia-sw: incoming request url: https://vitalik-eth.ipns.inbrowser.dev/css/misc.css: +0ms

Because these are all handled concurrently when the block isn't retrieved and cached yet in the memory block store, the block is requested multiple times.

FYI I think ipfs/helia#483

@SgtPooki Are you referring to any specific PR from that release?

& ipfs/helia-verified-fetch#50 resolves this.

My understanding is that sessions as implemented by ipfs/helia-verified-fetch#50 scope related requests to a specific set of block brokers. Having said that, I don't see how that would solve this problem where essentially 5 requests from the same root CID that share the path segment css are made concurrently with Verified Fetch.

I may be wrong, though.

@SgtPooki
Copy link
Member

There is some logic in helia sessions that append requests to inflight sessions based on root CID.

My thought is that vfetch is kicking off multiple requests for the root because multiple subresources that need loaded (like you mention), and the session code would handle new "wants" by appending to inflight "want-sessions"

@SgtPooki
Copy link
Member

It is worth mentioning that even without sessions we should have a way to handle duplicate block requests though.

@2color
Copy link
Member

2color commented Apr 18, 2024

My thought is that vfetch is kicking off multiple requests for the root because multiple subresources that need loaded (like you mention), and the session code would handle new "wants" by appending to inflight "want-sessions"

This may be the case for the Bitswap BlockBroker, but for the default Verified Fetch which uses helia/http, I couldn't find any logic that would handle this.

It is worth mentioning that even without sessions we should have a way to handle duplicate block requests though.

I agree.

@SgtPooki
Copy link
Member

This may be the case for the Bitswap BlockBroker, but for the default Verified Fetch which uses helia/http, I couldn't find any logic that would handle this.

I believe that logic lives in ipfs/helia-verified-fetch#50 currently. but yes, we need something in trustless-gateay block broker that can determine whether to kick off a getRawBlock request or not. a Set of inflight requests mapped on root and gateway should work

@SgtPooki
Copy link
Member

ipfs/helia#503 will fix this once it is approved, merged, and released.

achingbrain pushed a commit to ipfs/helia that referenced this issue Apr 20, 2024
Fixes ipfs/service-worker-gateway#104

This PR fixes issues brought up in service-worker-gateway where sub-resources end up causing multiple requests to a trustless gateway for the root CID.
@SgtPooki SgtPooki reopened this Apr 22, 2024
@SgtPooki
Copy link
Member

we need to make sure we get the latest block-brokers into helia-verified-fetch, and then pull that in here before this is resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment