boxo icon indicating copy to clipboard operation
boxo copied to clipboard

gateway: evaluate switching back to standard http library

Open lidel opened this issue 7 months ago • 3 comments

boxo/gateway uses custom (forked) implementation of http.ServeContent, the code is in boxo/gateway/erve_http_content.go#httpServeContent

TLDR: we forked it in 2023 for reasons (afaik to facilitate streaming CARs?) but created surface for bugs:

  • Ref. https://github.com/ipfs/boxo/pull/922

May be good time to re-evaluate if it is cost-effective / feasible to switch back.

lidel avatar May 27 '25 14:05 lidel

@aschmahmann do you remember the rationale/issues from 2023? (if not fine, we can find it later)

lidel avatar May 27 '25 14:05 lidel

Don't recall everything offhand, but I think it was that http.ServeContent had some awkward assumptions around range requests and seeking that were either actually problematic or potentially problematic due to us breaking a contract even if at the time the implementation would've been fine. Things like:

  • Seeking to the end of the file to determine the size when if we only have the data we need (blocks or CAR requests) when we'd likely just pull this from the data in the root
  • Repeated seeks to the start of the file when in a streaming CAR response we can't seek back to the beginning. In practice since we'd preempt the content-type sniffing it'd probably end up ok as long as we added some logic to error appropriately it'd a little sketchy to rely on

IIRC these both really boiled down to the code wanting a readerseeker and returning CAR streams being a reader and not a readerseeker kind of behavior.

As a result my recollection was that the tradeoff went something like:

  1. Pretend that a reader is actually a readerseeker and then add defensive tests to try and cover the bases
  2. Copy paste the code to fix that and add tests to make sure we did it right... hopefully notice if any changes in Go's stdlib around the copied codepaths need to be upstreamed

We chose 2 since it seemed less likely to cause us surprise problems.... but then the tests didn't make their way into either here or gateway conformance which 🎉 allowed at least one bug to get through. Fine to revisit if you feel the calculus has changed.

aschmahmann avatar May 27 '25 17:05 aschmahmann

We managed to resolve #922 without bigger refactor, so parking this as lower priority, but keep issue open if someone has spare cycles to open a PR.

Some pointers / ideas

  • We want to remove the need for custom boxo/gateway/serve_http_content.go#httpServeContent with the upstream http.ServeContent.
  • Tests from .github/workflows/gateway-conformance.yml have to pass.
  • Ideas to explore:
    • have TwoQueueCache (frequency + recency) with lengths and content types to avoid seeking on every range request for deserialized UnixFS file
    • have wrapper that pretends to do the seeking to learn size, but if UnixFS, return value from root block instead

Marking as P2, but we can bump priority if more bugs are caused by our forked code.

lidel avatar May 28 '25 16:05 lidel