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

deps: latest @helia/verified-fetch & http range support testing #122

Merged
merged 15 commits into from
Mar 21, 2024

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Mar 16, 2024

Title

This PR is WIP utilizing the latest release of @helia/verified-fetch with the http range header support.

Description

This PR updates the service-worker-gateway to support range requests. The @helia/verified-fetch package has been updated to the latest release, which includes support for the range header.

Notes & open questions

PR is ready for review. things are working.

PR is in draft mode while I test these things out, but for now, the only commit is updating the packages and resolving the conflicts.

  • Is there additional work to ensure the range header is passed through?
    • A simple fetch in the dev console can return singular characters for the "hello world" CID without worrying about any code changes.
  • can we load bigbuckbunny.mp4 with the range header? are they sent and received properly?
  • can we do a byte range request for an identity CID and get only a single first/last/middle character back? (e2e test)

TODO:

Progress updates:

2024-03-16 00:32 UTC

  • Note: attempting two videos resulted in Brave rendering unusable video controls and video viewer, where no action did anything.
    • bigbuckbunny=bafybeidsp6fva53dexzjycntiucts57ftecajcn5omzfgjx57pqfy3kwbq
    • stockSkateboarder=bafkreiezuss4xkt5gu256vjccx7vocoksxk77vwmdrpwoumfbbxcy2zowq

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (41d2cff) to head (88a4cfb).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #122   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           35        35           
  Branches         5         5           
=========================================
  Hits            35        35           
Flag Coverage Δ
node 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TypeError: Failed to execute 'put' on 'Cache': Partial response (status code 206) is unsupported
@SgtPooki

This comment was marked as outdated.

@SgtPooki
Copy link
Member Author

@SgtPooki

This comment was marked as outdated.

@SgtPooki

This comment was marked as outdated.

@SgtPooki

This comment was marked as outdated.

@SgtPooki

This comment was marked as outdated.

@SgtPooki

This comment was marked as outdated.

@SgtPooki SgtPooki changed the title deps: latest @helia/verified-fetch deps: latest @helia/verified-fetch & http range support testing Mar 16, 2024
@SgtPooki

This comment was marked as outdated.

@SgtPooki

This comment was marked as outdated.

@SgtPooki

This comment was marked as outdated.

@SgtPooki
Copy link
Member Author

i think i got it. byte range is returning invalid byte string. i’ve been testing with bafybeid2sc4k5ynx5yo7wg23fzw2ni4xxuxi7kdq5q3lyurkvglh5wrwmi and hardcoding headers.. and got it fixed with the below

  if (headers['content-range'] === 'bytes 0-83216545/83216545') {
    headers['content-range'] = 'bytes 0-83216544/83216545'
  }

@SgtPooki
Copy link
Member Author

@SgtPooki
Copy link
Member Author

image

Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

self review

@SgtPooki SgtPooki marked this pull request as ready for review March 19, 2024 00:12
@SgtPooki SgtPooki requested review from lidel and 2color March 19, 2024 00:13
@SgtPooki
Copy link
Member Author

SgtPooki commented Mar 19, 2024

one issue that comes up when loading large videos (https://bafybeidsp6fva53dexzjycntiucts57ftecajcn5omzfgjx57pqfy3kwbq.ipfs.sw.sgtpooki.com/) is that the request seems to take a while to return. I believe we're waiting for the full content to be loaded, when instead, we should return quickly with minimal data..

I thought we would see a second request from the browser when it sees the content type, but the browser is waiting until the entire first response is done before it sends any additional requests. We can't cancel/abort the original request until the browser tells us to do so, because someone may want the entire content.

Opened #131 for this

@2color
Copy link
Member

2color commented Mar 19, 2024

one issue that comes up when loading large videos (https://bafybeidsp6fva53dexzjycntiucts57ftecajcn5omzfgjx57pqfy3kwbq.ipfs.sw.sgtpooki.com/) is that the request seems to take a while to return.

Are you referring to the verifiedFetch request from the SW?

@2color
Copy link
Member

2color commented Mar 19, 2024

Btw, after the initial loading of https://bafybeidsp6fva53dexzjycntiucts57ftecajcn5omzfgjx57pqfy3kwbq.ipfs.sw.sgtpooki.com/ I'm getting a 502 that is coming from the service worker.
Screenshot 2024-03-19 at 2 48 31 pm

If I attempt to load the config page I also get a 502: https://bafybeidsp6fva53dexzjycntiucts57ftecajcn5omzfgjx57pqfy3kwbq.ipfs.sw.sgtpooki.com/#ipfs-sw-config

@SgtPooki
Copy link
Member Author

SgtPooki commented Mar 19, 2024

one issue that comes up when loading large videos (bafybeidsp6fva53dexzjycntiucts57ftecajcn5omzfgjx57pqfy3kwbq.ipfs.sw.sgtpooki.com) is that the request seems to take a while to return.

Are you referring to the verifiedFetch request from the SW?

Yea, the first time verified fetch is called for the content, it loads

Btw, after the initial loading of bafybeidsp6fva53dexzjycntiucts57ftecajcn5omzfgjx57pqfy3kwbq.ipfs.sw.sgtpooki.com I'm getting a 502 that is coming from the service worker.

If I attempt to load the config page I also get a 502: bafybeidsp6fva53dexzjycntiucts57ftecajcn5omzfgjx57pqfy3kwbq.ipfs.sw.sgtpooki.com/#ipfs-sw-config

Which service worker is registered? When i load https://bafybeidsp6fva53dexzjycntiucts57ftecajcn5omzfgjx57pqfy3kwbq.ipfs.sw.sgtpooki.com/#ipfs-sw-config, and reload, it continues displaying the config page for me, so i think you may have an old SW registered?

@lidel
Copy link
Member

lidel commented Mar 19, 2024

Found DoH bug, filled ipfs/helia#474 (not a blocker, just fyi)

@SgtPooki
Copy link
Member Author

FYI: merging this in order to continue improvements moving forward. will work on #134 next

@SgtPooki SgtPooki merged commit b064d20 into main Mar 21, 2024
28 checks passed
@SgtPooki SgtPooki deleted the feat/byte-range-requests branch March 21, 2024 18:37
@lidel lidel mentioned this pull request Mar 15, 2024
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.

4 participants