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.

Sign up for GitHub

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

Jump to bottom

Stream request body instead of buffering it in memory #8084

Merged
merged 10 commits into from
Aug 15, 2023
Merged

Stream request body instead of buffering it in memory #8084

merged 10 commits into from
Aug 15, 2023

Conversation

hbgl
Copy link
Contributor

@hbgl hbgl commented Aug 15, 2023

Changes

  • Resolves Performance bug when uploading large amounts of data #7525.
  • Stream request body instead of buffering it in memory.
  • Relies on non-standard behavior in undici that accepts AsyncIterables as request bodies. Node uses undici under the hood, so this is (unofficially) supported.
  • Refactor header conversion to make it type-safe.

Testing

  • Existing test cases for sending request bodies.
  • Added another test case for posting large request bodies (256MB).

Docs

  • Internal bugfix. The change does not affect user's behavior.

@changeset-bot
Copy link

changeset-bot bot commented Aug 15, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 85a28e0

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Aug 15, 2023
ematipico
ematipico reviewed Aug 15, 2023
View reviewed changes
packages/astro/src/core/app/node.ts Outdated Show resolved Hide resolved
packages/astro/src/core/app/node.ts Show resolved Hide resolved
packages/astro/src/core/app/node.ts
Comment on lines +93 to +95
// The duplex property is required when using a ReadableStream or async
// iterable for the body. The type definitions do not include the duplex
// property because they are not up-to-date.
Copy link
Member

Choose a reason for hiding this comment

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

Is there an updated version that matches the types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is sadly no updated version with the correct types. There exists an issue DefinitelyTyped/DefinitelyTyped#60924 but no fix yet (see RequestInit type definition).

@ematipico ematipico requested review from matthewp and bluwy August 15, 2023 08:21
matthewp
matthewp approved these changes Aug 15, 2023
View reviewed changes
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

lgtm!

bluwy
bluwy approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Looks great!

@hbgl
Copy link
Contributor Author

hbgl commented Aug 15, 2023

I assume the failing check for windows-latest (node@16) (pull_request) is unrelated to the change. I just ran the tests locally on Windows and Node V16 and it passed.

@bluwy
Copy link
Member

bluwy commented Aug 15, 2023

Yeah sorry about that. That particular test has been flaky. I re-ran it and will merge it once it passes.

@ematipico ematipico merged commit 560e459 into withastro:main Aug 15, 2023
@astrobot-houston astrobot-houston mentioned this pull request Aug 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ematipico ematipico ematipico left review comments

@bluwy bluwy bluwy approved these changes

@matthewp matthewp matthewp approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Performance bug when uploading large amounts of data
4 participants
@hbgl @bluwy @matthewp @ematipico