refactor(fetch): reimplement fetch with hyper instead of reqwest #24237

seanmonstar · 2024-06-17T23:56:41Z

This re-implements ext/fetch using hyper and hyper-util, instead of reqwest.

I'm missing a couple pieces that likely need some work over in other repos.

port Proxy functionality
add decompression layer for the response bodies

CLAassistant · 2024-06-17T23:56:47Z

All committers have signed the CLA.

lucacasonato · 2024-06-18T10:56:23Z

ext/fetch/lib.rs

+  let user_agent = user_agent
+    .parse()
+    .map_err(|_| type_error("illegal characters in User-Agent"))?;


We could require the caller deals with the fact that user_agent should be a valid HeaderValue, and just pass HeaderValue to this function.

Yea, this type could be pushed out to callers. FWIW, this parsing was happening right here previously, when passing the string to reqwest::ClientBuilder::user_agent().

ext/fetch/lib.rs

seanmonstar · 2024-07-08T17:08:25Z

Looks like a spurious CI error:

 Creating symlink "D:\\a\\deno\\deno\\target\\debug\\gn_root" to "C:\\Users\\runneradmin\\.cargo\\registry\\src\\index.crates.io-6f17d22bba15001f\\v8-0.95.0"

  --- stderr
  thread 'main' panicked at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\v8-0.95.0\build.rs:851:46:
  symlink_dir failed: Os { code: 183, kind: AlreadyExists, message: "Cannot create a file when that file already exists." }

devsnek

not super familiar with this code but it looks reasonable.

bartlomieju

LGTM, let's get this in and iron out any problem in follow ups.

Thanks so much @seanmonstar!

…est (#24237)" This reverts commit f6fd661.

…est (#24237)" (#24574) This reverts commit f6fd661. I'm seeing a difference between canary and 1.45.2. In `deno-docs/reference_gen` I can't download dax when running `deno task types` ``` ~/src/deno-docs/reference_gen# deno upgrade --canary Looking up latest canary version Found latest version f6fd661 Downloading https://dl.deno.land/canary/f6fd6619e708a515831f707438368d81b0c9aa56/deno-aarch64-apple-darwin.zip Deno is upgrading to version f6fd661 Archive: /var/folders/9v/kys6gqns6kl8nksyn4l1f9v40000gn/T/.tmpb5lDnq/deno.zip inflating: deno Upgraded successfully ~/src/deno-docs/reference_gen# deno -v deno 1.45.2+f6fd661 ~/src/deno-docs/reference_gen# rm -rf /Users/ry/Library/Caches/deno ~/src/deno-docs/reference_gen# deno task types Task types deno task types:deno && deno task types:node Task types:deno deno run --allow-read --allow-write --allow-run --allow-env --allow-sys deno-docs.ts error: JSR package manifest for '@david/dax' failed to load. expected value at line 1 column 1 at file:///Users/ry/src/deno-docs/reference_gen/deno-docs.ts:2:15 ~/src/deno-docs/reference_gen# deno upgrade --version 1.45.2 Downloading https://github.com/denoland/deno/releases/download/v1.45.2/deno-aarch64-apple-darwin.zip Deno is upgrading to version 1.45.2 Archive: /var/folders/9v/kys6gqns6kl8nksyn4l1f9v40000gn/T/.tmp3R7uhF/deno.zip inflating: deno Upgraded successfully ~/src/deno-docs/reference_gen# rm -rf /Users/ry/Library/Caches/deno ~/src/deno-docs/reference_gen# deno task types Task types deno task types:deno && deno task types:node Task types:deno deno run --allow-read --allow-write --allow-run --allow-env --allow-sys deno-docs.ts Task types:node deno run --allow-read --allow-write=. --allow-env --allow-sys node-docs.ts ```

) This commit re-implements `ext/fetch` and all dependent crates using `hyper` and `hyper-util`, instead of `reqwest`. The reasoning is that we want to have greater control and access to low level `hyper` APIs when implementing `fetch` API as well as `node:http` module. --------- Co-authored-by: Bartek Iwańczuk <biwanczuk@gmail.com>

…est (#24237)" (#24574) This reverts commit f6fd661. I'm seeing a difference between canary and 1.45.2. In `deno-docs/reference_gen` I can't download dax when running `deno task types` ``` ~/src/deno-docs/reference_gen# deno upgrade --canary Looking up latest canary version Found latest version f6fd661 Downloading https://dl.deno.land/canary/f6fd6619e708a515831f707438368d81b0c9aa56/deno-aarch64-apple-darwin.zip Deno is upgrading to version f6fd661 Archive: /var/folders/9v/kys6gqns6kl8nksyn4l1f9v40000gn/T/.tmpb5lDnq/deno.zip inflating: deno Upgraded successfully ~/src/deno-docs/reference_gen# deno -v deno 1.45.2+f6fd661 ~/src/deno-docs/reference_gen# rm -rf /Users/ry/Library/Caches/deno ~/src/deno-docs/reference_gen# deno task types Task types deno task types:deno && deno task types:node Task types:deno deno run --allow-read --allow-write --allow-run --allow-env --allow-sys deno-docs.ts error: JSR package manifest for '@david/dax' failed to load. expected value at line 1 column 1 at file:///Users/ry/src/deno-docs/reference_gen/deno-docs.ts:2:15 ~/src/deno-docs/reference_gen# deno upgrade --version 1.45.2 Downloading https://github.com/denoland/deno/releases/download/v1.45.2/deno-aarch64-apple-darwin.zip Deno is upgrading to version 1.45.2 Archive: /var/folders/9v/kys6gqns6kl8nksyn4l1f9v40000gn/T/.tmp3R7uhF/deno.zip inflating: deno Upgraded successfully ~/src/deno-docs/reference_gen# rm -rf /Users/ry/Library/Caches/deno ~/src/deno-docs/reference_gen# deno task types Task types deno task types:deno && deno task types:node Task types:deno deno run --allow-read --allow-write --allow-run --allow-env --allow-sys deno-docs.ts Task types:node deno run --allow-read --allow-write=. --allow-env --allow-sys node-docs.ts ```

lucacasonato added the ci-draft Run the CI on draft PRs. label Jun 18, 2024

lucacasonato reviewed Jun 18, 2024

View reviewed changes

bartlomieju mentioned this pull request Jun 18, 2024

fix(ext/node): add ServerResponse#appendHeader #24216

Merged

bartlomieju reviewed Jun 18, 2024

View reviewed changes

ext/fetch/lib.rs Outdated Show resolved Hide resolved

refactor: reimplement fetch with hyper instead of reqwest

9414b37

seanmonstar force-pushed the fetch-with-hyper branch from c42c5b2 to 9414b37 Compare July 2, 2024 15:44

seanmonstar added 3 commits July 2, 2024 11:53

add request_builder_hook back

b3a969d

add copyright to proxy.rs file

7ebe019

send user-agent with proxy tunnel request

a652549

seanmonstar marked this pull request as ready for review July 2, 2024 16:13

seanmonstar and others added 19 commits July 2, 2024 16:18

fix https tunneling, add back socks

a86ca03

format imports

9e46f21

fix proxy parsing unit test

3108c3c

format unit tests

ae7bba9

clippy the fetch crate

ab54f12

format after clippy

6218613

clippy and fmt kv/cli

c435533

add reqwest dev-dependency; fix tests

f1b03dd

Merge branch 'main' into fetch-with-hyper

349bb41

use workspace reqwest

6a274bc

fix copyright

29e4aee

fix proxy parse from env test

c959ea8

fix non-proxy to not send absolute-form

571944c

fetch tests: remove space between gzip, br

04da78a

tests: update error message assertions

b6b12f4

tests: fix serve_test error message assertion

e8d746e

Merge remote-tracking branch 'upstream/main' into fetch-with-hyper

c6b4070

tests: fix async error stack pattern assertion

43f2cb9

macos: remove rustls-native-certs dependency

693ae6d

seanmonstar and others added 11 commits July 3, 2024 19:35

extract hyper body error source again

f3beefa

use a ResourceBodyError sentinel type

d6d3dc2

Merge remote-tracking branch 'upstream/main' into fetch-with-hyper

2148659

remove ResourceBodyError wrapper, it didn't work

b6aaf04

Merge branch 'main' into fetch-with-hyper

92ffdf7

lint

8ccf6c7

send proxy-authorization when proxy is http forward

b66e01a

Merge remote-tracking branch 'upstream/main' into fetch-with-hyper

2a75c2b

fmt

31994c0

Merge remote-tracking branch 'upstream/main' into fetch-with-hyper

d2a4f2b

Merge remote-tracking branch 'upstream/main' into fetch-with-hyper

2354a6e

seanmonstar and others added 8 commits July 8, 2024 15:34

Merge remote-tracking branch 'upstream/main' into fetch-with-hyper

434b431

reset CI

673deb8

Merge branch 'main' into fetch-with-hyper

e35038f

Merge branch 'main' into fetch-with-hyper

5eb1fdf

allow proxy tunnel response to have more headers

58efd4d

Merge branch 'main' into fetch-with-hyper

04ff732

Merge branch 'main' into fetch-with-hyper

ee6526c

gh cache bust

4db5c39

bartlomieju mentioned this pull request Jul 11, 2024

HTTP/1.1: Chunked message bodies incorrectly terminated on \r\n\r\n instead of 0\r\n\r\n #24455

Open

devsnek approved these changes Jul 11, 2024

View reviewed changes

Merge branch 'main' into fetch-with-hyper

7f064c5

bartlomieju approved these changes Jul 12, 2024

View reviewed changes

bartlomieju merged commit f6fd661 into denoland:main Jul 12, 2024
17 checks passed

ry added a commit that referenced this pull request Jul 13, 2024

Revert "refactor(fetch): reimplement fetch with hyper instead of reqw…

6b77400

…est (#24237)" This reverts commit f6fd661.

seanmonstar deleted the fetch-with-hyper branch July 24, 2024 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(fetch): reimplement fetch with hyper instead of reqwest #24237

refactor(fetch): reimplement fetch with hyper instead of reqwest #24237

seanmonstar commented Jun 17, 2024 •

edited

Loading

CLAassistant commented Jun 17, 2024 •

edited

Loading

lucacasonato Jun 18, 2024

seanmonstar Jul 2, 2024

seanmonstar commented Jul 8, 2024

devsnek left a comment

bartlomieju left a comment

refactor(fetch): reimplement fetch with hyper instead of reqwest #24237

refactor(fetch): reimplement fetch with hyper instead of reqwest #24237

Conversation

seanmonstar commented Jun 17, 2024 • edited Loading

CLAassistant commented Jun 17, 2024 • edited Loading

lucacasonato Jun 18, 2024

Choose a reason for hiding this comment

seanmonstar Jul 2, 2024

Choose a reason for hiding this comment

seanmonstar commented Jul 8, 2024

devsnek left a comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

seanmonstar commented Jun 17, 2024 •

edited

Loading

CLAassistant commented Jun 17, 2024 •

edited

Loading