feat: abort signals are respected #26

SgtPooki · 2024-03-21T02:36:42Z

Title

feat: abort signals are respected

Description

fixes #25

need some eyes on this to ensure the approach is sound and that we're exiting as early as possible.

Notes & open questions

slightly WIP but I do need feedback, so PR not marked as WIP.

There was an obvious bug where signal was not being passed through fully. :

no signal being read at all (it was always undefined): see the changes to convertOptions
parse-resource did not previously accept a signal.
parse-url-string did not previously accept a signal.

I'm happy to revert some of the explicit signal handling and just get through the fixes to the above, but I think we should exit early when we can.

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

- badRequestResponse is called correctly - badRequestResponse accepts error for body - add tests for abort handling

packages/verified-fetch/src/utils/parse-url-string.ts

packages/verified-fetch/src/verified-fetch.ts

SgtPooki · 2024-03-21T02:40:59Z

packages/verified-fetch/src/verified-fetch.ts

+   *
+   * TODO: Should we use 400, 408, 418, or 425, or throw and not even return a response?


we should discuss this.

packages/verified-fetch/src/verified-fetch.ts

SgtPooki · 2024-03-21T02:42:26Z

packages/verified-fetch/src/verified-fetch.ts

  async fetch (resource: Resource, opts?: VerifiedFetchOptions): Promise<Response> {
    this.log('fetch %s', resource)

    const options = convertOptions(opts)

+    const opController = new AbortController()
+    if (options?.signal != null) {
+      options.signal.onabort = this.abortHandler.bind(this, opController)


we override any handlers they have on their signal.. they should wrap any signals they want to provide

we can change this later if we want.. but it would be good to be aware of this. @achingbrain

packages/verified-fetch/test/fixtures/dns-answer-fake.ts

SgtPooki

self review

SgtPooki

self review.. maybe some duplicated comments.

packages/verified-fetch/test/fixtures/dns-answer-fake.ts

SgtPooki · 2024-03-21T21:00:13Z

packages/verified-fetch/src/verified-fetch.ts

@@ -78,7 +78,10 @@ function convertOptions (options?: VerifiedFetchOptions): (Omit<VerifiedFetchOpt
  let signal: AbortSignal | undefined
  if (options?.signal === null) {
    signal = undefined
+  } else {
+    signal = options?.signal


the main part that fixes the bug mentioned by #25

SgtPooki · 2024-03-21T21:00:35Z

packages/verified-fetch/src/utils/parse-resource.ts


 export interface ParseResourceComponents {
  ipns: IPNS
  logger: ComponentLogger
 }

-export interface ParseResourceOptions extends ProgressOptions<ResolveProgressEvents | IPNSRoutingEvents | ResolveDNSLinkProgressEvents> {
+export interface ParseResourceOptions extends ParseUrlStringOptions {


we extend options accepted by ParseUrlStringOptions

SgtPooki · 2024-03-21T21:03:41Z

packages/verified-fetch/src/utils/get-stream-from-async-iterable.ts

@@ -5,7 +5,7 @@ import type { ComponentLogger } from '@libp2p/interface'
 /**
 * Converts an async iterator of Uint8Array bytes to a stream and returns the first chunk of bytes
 */
-export async function getStreamFromAsyncIterable (iterator: AsyncIterable<Uint8Array>, path: string, logger: ComponentLogger, options?: Pick<VerifiedFetchInit, 'onProgress'>): Promise<{ stream: ReadableStream<Uint8Array>, firstChunk: Uint8Array }> {
+export async function getStreamFromAsyncIterable (iterator: AsyncIterable<Uint8Array>, path: string, logger: ComponentLogger, options?: Pick<VerifiedFetchInit, 'onProgress' | 'signal'>): Promise<{ stream: ReadableStream<Uint8Array>, firstChunk: Uint8Array }> {


another part of the bug fix... we need to be passing progressOptions and signal through. I just realized I wasn't checking for signal abort here... the iterator should handle this.. but i'll update just in case.

SgtPooki · 2024-03-21T21:04:07Z

packages/verified-fetch/src/utils/parse-url-string.ts

@@ -13,7 +13,7 @@ export interface ParseUrlStringInput {
  ipns: IPNS
  logger: ComponentLogger
 }
-export interface ParseUrlStringOptions extends ProgressOptions<ResolveDNSLinkProgressEvents> {
+export interface ParseUrlStringOptions extends ProgressOptions<ResolveProgressEvents | IPNSRoutingEvents | ResolveDNSLinkProgressEvents>, AbortOptions {


another part of the bug fix. we should accept a {signal}

SgtPooki

reviewed last changes. will merge shortly

SgtPooki · 2024-03-21T21:07:21Z

packages/verified-fetch/src/utils/get-stream-from-async-iterable.ts

+      if (options?.signal?.aborted === true) {
+        controller.error(new Error(options.signal.reason ?? 'signal aborted by user'))
+        controller.close()
+        return
+      }


added in 3b58fa0 (#26).

iterator from unixfs.cat should handle failing this stream, but handling this explicitly may allow us to exit earlier

## @helia/verified-fetch [1.3.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-1.2.1...@helia/verified-fetch-1.3.0) (2024-03-21) ### Features * abort signals are respected ([#26](#26)) ([30148fe](30148fe)) ### Bug Fixes * set cache-control header correctly ([#19](#19)) ([114f3a4](114f3a4)), closes [#17](#17) [#23](#23) [#10](#10)

github-actions · 2024-03-21T21:26:13Z

🎉 This PR is included in version 1.3.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

github-actions · 2024-03-21T21:26:30Z

🎉 This PR is included in version 1.9.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

github-actions · 2024-05-09T21:51:32Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

## 1.0.0 (2024-10-14) ### ⚠ BREAKING CHANGES * upgrade to helia v5 (#107) ### Features * abort signals are respected ([#26](#26)) ([30148fe](30148fe)) * allow use as a singleton ([#413](https://github.com/ipfs/helia-verified-fetch/issues/413)) ([7cbeed0](7cbeed0)) * create @helia/verified-fetch ([#392](https://github.com/ipfs/helia-verified-fetch/issues/392)) ([06d36fe](06d36fe)) * customize ipns dnsResolvers ([#445](https://github.com/ipfs/helia-verified-fetch/issues/445)) ([8f60822](8f60822)) * download tars from @helia/verified-fetch ([#442](https://github.com/ipfs/helia-verified-fetch/issues/442)) ([70ddd00](70ddd00)) * require content-type parser to set content-type ([#423](https://github.com/ipfs/helia-verified-fetch/issues/423)) ([b39d07c](b39d07c)) * support `Accept` header in `@helia/verified-fetch` ([#438](https://github.com/ipfs/helia-verified-fetch/issues/438)) ([54c4383](54c4383)) * support downloading car files from @helia/verified-fetch ([#441](https://github.com/ipfs/helia-verified-fetch/issues/441)) ([703980c](703980c)) * support http range header ([#10](#10)) ([9f5078a](9f5078a)) * support IPFS/IPNS paths, Gateways, etc ([#4](#4)) ([e7f1816](e7f1816)) * support redirects for UnixFS directories ([#5](#5)) ([4601d46](4601d46)) * support requesting raw IPNS records in @helia/verified-fetch ([#443](https://github.com/ipfs/helia-verified-fetch/issues/443)) ([e92086a](e92086a)) * upgrade to helia v5 ([#107](#107)) ([91a6473](91a6473)) * use blockstore sessions ([#50](#50)) ([541dd64](541dd64)) * use the waterworks trustless gateway by default ([#94](#94)) ([2db8d1e](2db8d1e)) ### Bug Fixes * @helia/verified-fetch init args are optional ([#412](https://github.com/ipfs/helia-verified-fetch/issues/412)) ([e6d41a0](e6d41a0)) * aborted signals throw libp2p AbortError ([#30](#30)) ([4575791](4575791)) * add interop tests and update project config ([fdc83b8](fdc83b8)) * allow contentTypeParser with Helia instance ([#427](https://github.com/ipfs/helia-verified-fetch/issues/427)) ([b5c95e6](b5c95e6)) * append query path to path resolved from IPNS name ([#3](#3)) ([fd86e6a](fd86e6a)) * byte range request end should never equal file size ([#24](#24)) ([aafc567](aafc567)) * content-type response header hints how to process response ([#426](https://github.com/ipfs/helia-verified-fetch/issues/426)) ([8b78e79](8b78e79)) * decodeURIComponent on path parts ([#40](#40)) ([f628cf6](f628cf6)) * do not coerce `undefined` to `null` for JSON serialization ([#2](#2)) ([d36ce29](d36ce29)) * gateway conformance improvements ([#85](#85)) ([7281078](7281078)) * gateway conformance tests ([#81](#81)) ([d0a3b6b](d0a3b6b)) * identity CIDs use contentTypeParser ([#49](#49)) ([3014498](3014498)) * implicit accept header can be overridden by format query ([#36](#36)) ([75c0b75](75c0b75)) * missing paths returns 404 instead of 502 ([#59](#59)) ([291a054](291a054)), closes [#53](#53) * pass url and body to badRequestResponse ([#44](#44)) ([cc228e6](cc228e6)) * prevent interop timeouts with fast fixture loading ([#73](#73)) ([a43d994](a43d994)) * reduce dagPb and dagCbor handler complexity ([#45](#45)) ([3b41752](3b41752)) * remove redundant abort controller ([#41](#41)) ([04b220d](04b220d)) * remove stubs from verified-fetch tests ([#431](https://github.com/ipfs/helia-verified-fetch/issues/431)) ([cbab8b0](cbab8b0)) * set cache-control header correctly ([#19](#19)) ([114f3a4](114f3a4)), closes [#17](#17) [#23](#23) [#10](#10) * support https?://<dnsLink>.ipns.<gateway> urls ([#16](#16)) ([0ece19a](0ece19a)) * ttl and caching for ipns urls ([#34](#34)) ([44ac5a1](44ac5a1)) * unixfs dir redirect ([#33](#33)) ([32ca87f](32ca87f)) * update @helia/ipns and dns config ([#18](#18)) ([9f88c54](9f88c54)) * use CID object for verified-fetch progress events ([#425](https://github.com/ipfs/helia-verified-fetch/issues/425)) ([b010184](b010184)) * use ipfs-unixfs-exporter directly ([#42](#42)) ([4532bf1](4532bf1)) * verified-fetch etag header ([#434](https://github.com/ipfs/helia-verified-fetch/issues/434)) ([f9b1ffe](f9b1ffe)) * verified-fetch header cache-control typo ([#433](https://github.com/ipfs/helia-verified-fetch/issues/433)) ([ea39b48](ea39b48)) * walking dag-cbor paths ([#39](#39)) ([99668ce](99668ce)) * X-Ipfs-Path is set correctly ([#46](#46)) ([5bb6685](5bb6685)) ### Trivial Changes * add tests for directory redirects for gateways ([#15](#15)) ([269609d](269609d)) * fix build ([#22](#22)) ([01261fe](01261fe)) * initial import ([4b4fb8c](4b4fb8c)) * **release:** 1.0.0 [skip ci] ([ef5b6a5](ef5b6a5)), closes [#68](#68) * **release:** 1.0.0 [skip ci] ([a584a1a](a584a1a)) * **release:** 1.0.0 [skip ci] ([5207a64](5207a64)) * **release:** 1.0.1 [skip ci] ([5499724](5499724)), closes [#81](#81) * **release:** 1.0.1 [skip ci] ([8d2fdda](8d2fdda)), closes [#2](#2) * **release:** 1.0.2 [skip ci] ([104af2d](104af2d)), closes [#3](#3) * **release:** 1.1.0 [skip ci] ([e300af2](e300af2)) * **release:** 1.1.0 [skip ci] ([f4fcb4d](f4fcb4d)), closes [#4](#4) [#5](#5) [#6](#6) * **release:** 1.1.0 [skip ci] ([b9b959b](b9b959b)) * **release:** 1.1.1 [skip ci] ([8ff0b8b](8ff0b8b)), closes [#83](#83) * **release:** 1.1.1 [skip ci] ([6f2c6bd](6f2c6bd)), closes [#15](#15) [#14](#14) * **release:** 1.1.2 [skip ci] ([6161aba](6161aba)), closes [#85](#85) * **release:** 1.1.2 [skip ci] ([4e1abc0](4e1abc0)), closes [#16](#16) * **release:** 1.1.3 [skip ci] ([14c2302](14c2302)), closes [#18](#18) * **release:** 1.10.0 [skip ci] ([4589c26](4589c26)) * **release:** 1.10.1 [skip ci] ([140bab3](140bab3)), closes [#33](#33) * **release:** 1.11.0 [skip ci] ([39bb14e](39bb14e)) * **release:** 1.12.0 [skip ci] ([50682bf](50682bf)) * **release:** 1.13.0 [skip ci] ([e00d41c](e00d41c)) * **release:** 1.14.0 [skip ci] ([2521838](2521838)) * **release:** 1.15.0 [skip ci] ([a38a0c6](a38a0c6)) * **release:** 1.15.1 [skip ci] ([f59e862](f59e862)), closes [#47](#47) * **release:** 1.16.0 [skip ci] ([0a18c44](0a18c44)) * **release:** 1.17.0 [skip ci] ([f5299cf](f5299cf)) * **release:** 1.18.0 [skip ci] ([7771af2](7771af2)) * **release:** 1.19.0 [skip ci] ([089635d](089635d)) * **release:** 1.2.0 [skip ci] ([befd270](befd270)) * **release:** 1.2.0 [skip ci] ([e836abf](e836abf)), closes [#10](#10) [#22](#22) * **release:** 1.2.0 [skip ci] ([77d5e9e](77d5e9e)) * **release:** 1.2.1 [skip ci] ([718644b](718644b)), closes [#24](#24) * **release:** 1.20.0 [skip ci] ([abaaeab](abaaeab)) * **release:** 1.21.0 [skip ci] ([aeb5a89](aeb5a89)) * **release:** 1.21.1 [skip ci] ([f7ac2e7](f7ac2e7)), closes [#73](#73) * **release:** 1.22.0 [skip ci] ([6d4b26c](6d4b26c)), closes [#50](#50) * **release:** 1.23.0 [skip ci] ([754e219](754e219)) * **release:** 1.24.0 [skip ci] ([c1a795f](c1a795f)) * **release:** 1.24.1 [skip ci] ([9f8fb86](9f8fb86)), closes [#85](#85) [#54](#54) * **release:** 1.25.0 [skip ci] ([3d8bb78](3d8bb78)) * **release:** 1.3.0 [skip ci] ([43c41e4](43c41e4)), closes [#26](#26) [#17](#17) [#23](#23) [#10](#10) * **release:** 1.3.0 [skip ci] ([bf8af38](bf8af38)) * **release:** 1.3.1 [skip ci] ([0bf5847](0bf5847)), closes [#30](#30) * **release:** 1.3.10 [skip ci] ([6768fbd](6768fbd)), closes [#51](#51) * **release:** 1.3.11 [skip ci] ([8606b15](8606b15)) * **release:** 1.3.12 [skip ci] ([969a2e0](969a2e0)), closes [#42](#42) * **release:** 1.3.13 [skip ci] ([a180547](a180547)), closes [#53](#53) [#46](#46) * **release:** 1.3.14 [skip ci] ([31f238a](31f238a)), closes [#63](#63) * **release:** 1.3.2 [skip ci] ([2e141ca](2e141ca)), closes [#33](#33) * **release:** 1.3.3 [skip ci] ([19574cf](19574cf)), closes [#34](#34) * **release:** 1.3.4 [skip ci] ([7b06a56](7b06a56)), closes [#40](#40) * **release:** 1.3.5 [skip ci] ([554bb3e](554bb3e)), closes [#41](#41) * **release:** 1.3.6 [skip ci] ([489a1d6](489a1d6)), closes [#36](#36) * **release:** 1.3.7 [skip ci] ([d0114c1](d0114c1)), closes [#39](#39) * **release:** 1.3.8 [skip ci] ([0a37856](0a37856)), closes [#44](#44) [#47](#47) * **release:** 1.3.9 [skip ci] ([768e3ff](768e3ff)), closes [#49](#49) * **release:** 1.4.0 [skip ci] ([dab598a](dab598a)), closes [#50](#50) * **release:** 1.4.0 [skip ci] ([e008b20](e008b20)) * **release:** 1.4.1 [skip ci] ([e3c0a68](e3c0a68)), closes [#68](#68) [#72](#72) [#72](#72) * **release:** 1.4.2 [skip ci] ([2633d31](2633d31)), closes [#45](#45) * **release:** 1.4.3 [skip ci] ([924a6df](924a6df)), closes [#85](#85) * **release:** 1.5.0 [skip ci] ([4885fde](4885fde)), closes [#94](#94) * **release:** 1.5.0 [skip ci] ([71d613d](71d613d)) * **release:** 1.5.1 [skip ci] ([4e6775c](4e6775c)), closes [#12](#12) * **release:** 1.6.0 [skip ci] ([03d0409](03d0409)) * **release:** 1.7.0 [skip ci] ([8bf9c9f](8bf9c9f)) * **release:** 1.8.0 [skip ci] ([a240056](a240056)) * **release:** 1.9.0 [skip ci] ([8715740](8715740)) * start running CI again ([#64](#64)) ([9399f95](9399f95)) * stub blocks interface in tests ([#418](https://github.com/ipfs/helia-verified-fetch/issues/418)) ([a06f64b](a06f64b)) * unskip IPNS test ([#6](#6)) ([76485a4](76485a4)) * Update .github/pull_request_template.md [skip ci] ([5b825e6](5b825e6)) * Update .github/workflows/stale.yml [skip ci] ([3e40ad0](3e40ad0)) * update project config ([#419](https://github.com/ipfs/helia-verified-fetch/issues/419)) ([cb9dc90](cb9dc90)) ### Documentation * add link to blog post and ready-to-run example ([#63](#63)) ([696ed57](696ed57)) * add link to the blog post and example ([#87](#87)) ([f30f979](f30f979)) * change cidv0 to cidv1 in the readme ([#105](#105)) ([060e726](060e726)) * fix readme link ([#51](#51)) ([8a41c57](8a41c57)) * generate readme ([#70](#70)) ([a75567a](a75567a)), closes [#68](#68) [#72](#72) [#72](#72) * move the package docs to root of repo ([#1](#1)) ([8fb4a76](8fb4a76)) * update api docs link in readme ([#14](#14)) ([d615633](d615633)) * update documented default value ([161a470](161a470)), closes [/github.com/multiformats/js-dns/blob/a56c9e0b953d644392cf10fd0792757da0d61c32/src/resolvers/default.browser.ts#L6-L7](https://github.com/ipfs//github.com/multiformats/js-dns/blob/a56c9e0b953d644392cf10fd0792757da0d61c32/src/resolvers/default.browser.ts/issues/L6-L7) ### Dependencies * bump aegir from 42.2.11 to 44.1.4 ([#108](#108)) ([e36fbff](e36fbff)) * bump kubo from 0.26.0 to 0.27.0 ([#12](#12)) ([92cad49](92cad49)) * bump kubo from 0.27.0 to 0.28.0 ([#54](#54)) ([3579844](3579844)) * updating all deps ([#47](#47)) ([6d0ffd8](6d0ffd8)) ### Tests * add gateway conformance tests ([#67](#67)) ([30958fb](30958fb)), closes [#68](#68) * use successRate for gateway conformance tests ([#83](#83)) ([5f71a33](5f71a33))

github-actions · 2024-10-14T10:04:02Z

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

SgtPooki added 2 commits March 20, 2024 16:31

fix: options.signal should be used if not === null

dc2a768

fix: misc

565c321

- badRequestResponse is called correctly - badRequestResponse accepts error for body - add tests for abort handling

SgtPooki requested a review from a team as a code owner March 21, 2024 02:36

SgtPooki linked an issue Mar 21, 2024 that may be closed by this pull request

Abort signal is not respected #25

Closed

SgtPooki commented Mar 21, 2024

View reviewed changes

packages/verified-fetch/src/utils/parse-url-string.ts Outdated Show resolved Hide resolved

SgtPooki commented Mar 21, 2024

View reviewed changes

packages/verified-fetch/src/verified-fetch.ts Outdated Show resolved Hide resolved

SgtPooki commented Mar 21, 2024

View reviewed changes

packages/verified-fetch/src/verified-fetch.ts Outdated Show resolved Hide resolved

SgtPooki commented Mar 21, 2024

View reviewed changes

packages/verified-fetch/src/verified-fetch.ts Outdated Show resolved Hide resolved

SgtPooki commented Mar 21, 2024

View reviewed changes

packages/verified-fetch/test/fixtures/dns-answer-fake.ts Show resolved Hide resolved

SgtPooki commented Mar 21, 2024

View reviewed changes

SgtPooki added 7 commits March 21, 2024 11:58

Merge branch 'main' into 25-abort-signal-is-not-respected

5d222a8

chore: remove throwIfAborted expressions

f03be52

chore: significant cleanup of abort handling

59686df

chore: remove debugging log statement

043490f

chore: move test fixtures

d3dc777

chore: remove commented out test fixture

41b8e04

fix: getStreamFromAsyncIterable closes on signal abort

3b58fa0

SgtPooki commented Mar 21, 2024

View reviewed changes

SgtPooki self-assigned this Mar 21, 2024

SgtPooki merged commit 30148fe into main Mar 21, 2024
23 checks passed

SgtPooki deleted the 25-abort-signal-is-not-respected branch March 21, 2024 21:14

github-actions bot added the released label Mar 21, 2024

This was referenced Mar 21, 2024

bug: badRequestResponse is being called incorrectly #27

Closed

feat: quick TTFB & TTI for large content ipfs/service-worker-gateway#138

Merged

SgtPooki mentioned this pull request Mar 27, 2024

fix: helia-docker container dies when running in tiros ipfs/helia-http-gateway#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: abort signals are respected #26

feat: abort signals are respected #26

SgtPooki commented Mar 21, 2024 •

edited

Loading

SgtPooki Mar 21, 2024

SgtPooki Mar 21, 2024

SgtPooki Mar 21, 2024

SgtPooki left a comment

SgtPooki left a comment

SgtPooki Mar 21, 2024

SgtPooki Mar 21, 2024

SgtPooki Mar 21, 2024

SgtPooki Mar 21, 2024

SgtPooki left a comment

SgtPooki Mar 21, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented May 9, 2024

github-actions bot commented Oct 14, 2024

		*
		* TODO: Should we use 400, 408, 418, or 425, or throw and not even return a response?

feat: abort signals are respected #26

feat: abort signals are respected #26

Conversation

SgtPooki commented Mar 21, 2024 • edited Loading

Title

Description

Notes & open questions

Change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki left a comment

Choose a reason for hiding this comment

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

github-actions bot commented May 9, 2024

github-actions bot commented Oct 14, 2024

SgtPooki commented Mar 21, 2024 •

edited

Loading