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

refactor: use resourceForReadableStream for fetch #20217

Merged
merged 6 commits into from
Dec 1, 2023

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Aug 20, 2023

Switch ext/fetch over to resourceForReadableStream to simplify and unify implementation with ext/serve. This allows us to work in Rust with resources only.

Two additional changes made to resourceForReadableStream were required:

  • Add an optional length to resourceForReadableStream which translates to size_hint
  • Fix a bug where writing to a closed stream that was full would panic

ext/fetch/lib.rs Outdated
@@ -214,6 +212,58 @@ pub fn get_or_create_client_from_state(
}
}

struct ResourceToBodyAdapter(Rc<dyn Resource>, Option<Pin<Box<dyn Future<Output = Result<BufView, Error>>>>>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pattern is identical to one in ext/http, so it might be worth turning this into a deno_core utility.

@mmastrac mmastrac force-pushed the fetch_new_stream branch 2 times, most recently from 57431bc to 5813961 Compare September 7, 2023 19:45
@bartlomieju bartlomieju changed the title [WIP] use resourceForReadableStream for fetch refactor: use resourceForReadableStream for fetch Nov 8, 2023
@mmastrac mmastrac force-pushed the fetch_new_stream branch 2 times, most recently from 963c44c to ed19140 Compare November 30, 2023 20:16
Copy link
Member

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

LGTM! Great to have this finally land

@mmastrac mmastrac merged commit e6e708e into denoland:main Dec 1, 2023
14 checks passed
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.

fetch() rejection intermittently causes Deno to panic in threadsafe_functions.rs:113
2 participants