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

Cancel response stream when connection closes #9071

Merged
merged 4 commits into from Nov 15, 2023
Merged

Cancel response stream when connection closes #9071

merged 4 commits into from Nov 15, 2023

Conversation

pilcrowOnPaper
Copy link
Contributor

Changes

Testing

No test added, would love some directions

Docs

None

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 12, 2023 •
edited

🦋 Changeset detected

Latest commit: d2f50f2

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) docs pr A PR that includes documentation for review labels Nov 12, 2023
@pilcrowOnPaper pilcrowOnPaper changed the title cancel stream on close Cancel response stream when connection closes Nov 12, 2023
@lilnasy
Copy link
Contributor

lilnasy commented Nov 12, 2023

Thanks for the enhancement, the code looks great! Good eye on the unused modules as well.

Do you think this behavior could be easily tested?

@pilcrowOnPaper
Copy link
Contributor Author

@lilnasy Hm, it should be possible, though I'm not sure what's the best way to test step 4

  1. Make fetch() request to server
  2. Return response with stream
  3. Abort fetch() request
  4. Check if cancel() was called

@lilnasy
Copy link
Contributor

lilnasy commented Nov 12, 2023

What is the current unwanted behavior?

Is it that the readable stream continues to produce chunks, possibly endlessly?

@pilcrowOnPaper
Copy link
Contributor Author

Is it that the readable stream continues to produce chunks, possibly endlessly?

Yeah. This is especially an issue for server sent events since there's no definite end to the stream. It continues to push data until the client closes the connection

@lilnasy
Copy link
Contributor

lilnasy commented Nov 12, 2023

Yeah, we could use locals for that. I'll take a look.

lilnasy
lilnasy approved these changes Nov 12, 2023
View reviewed changes
Copy link
Contributor

@lilnasy lilnasy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

bluwy
bluwy approved these changes Nov 14, 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.

The original PR that added the response iterator was #5418, but it sounds like this PR aligns more with how Response work as a standard, so I think this is the right approach.

@pilcrowOnPaper
Copy link
Contributor Author

Is it true that the response body can be a Buffer?

Sometimes Astro sends a ReadableStream or a Buffer as a response body

@bluwy
Copy link
Member

bluwy commented Nov 14, 2023

I think it's possible in the past when we support returning simple objects in endpoints, but now that we enforce and coerce to Response, it shouldn't apply now.

@pilcrowOnPaper @sarah11918
Update .changeset/modern-ways-develop.md
d2f50f2 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@lilnasy lilnasy merged commit c948713 into withastro:main Nov 15, 2023
1 check passed
This was referenced Nov 15, 2023
Merged
Merged
peng added a commit to peng/astro that referenced this pull request Nov 17, 2023
@peng
Merge branch 'main' into feat-add-warn
cd112c9 
* main:
  feat(i18n): add `Astro.currentLocale` (withastro#9101)
  [ci] release (withastro#9107)
  Add compatibility with cloudflare node (withastro#8925)
  [ci] format
  Cancel response stream when connection closes (withastro#9071)
  [ci] format
  feat(i18n): apply specific routing logic only to pages (withastro#9091)
  feat(dev-overlay): Hide plugins into a separate menu when there's too many enabled (withastro#9102)
  [ci] format
  Support Svelte 5 (experimental) (withastro#9098)
  [ci] release (withastro#9078)
  [ci] format
  Refactor shikiji syntax highlighting code (withastro#9083)
  [ci] format
  fix: Query params trigger the trailingSlash error in preview mode (withastro#9045)
  fix(assets): bundling regression for specific config on non-Node runtimes (withastro#9087)
natemoo-re pushed a commit that referenced this pull request Nov 22, 2023
@pilcrowOnPaper @lilnasy
@sarah11918 @natemoo-re
Cancel response stream when connection closes (#9071)
79c5f31 
* cancel stream on close

* add changeset

* add test

* Update .changeset/modern-ways-develop.md

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

---------

Co-authored-by: lilnasy <69170106+lilnasy@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@lilnasy lilnasy mentioned this pull request Nov 30, 2023
Closed
1 task
@mingjunlu mingjunlu mentioned this pull request Feb 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sarah11918 sarah11918 sarah11918 left review comments

@bluwy bluwy bluwy approved these changes

@lilnasy lilnasy lilnasy approved these changes

Assignees
No one assigned
Labels
docs pr A PR that includes documentation for review pkg: integration Related to any renderer integration (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ReadableStream in response body not canceled when client closes connection
4 participants
@pilcrowOnPaper @lilnasy @bluwy @sarah11918