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

fix(streams): prevent earlyZipReadableStreams() from possibly using excessive memory #5082

Merged

Conversation

BlackAsLight
Copy link
Contributor

Jumping from: #5078 (comment)

This pull request changes the structure of the readable stream to use a pull method instead of the start method, stopping it from the possibility of using excessive memory, possibly overflowing it if not consumed fast enough, or at all. This alternative also offers a way to cancel the provided reason streams from the returned stream.

Current Version - overloaded the memory

Screenshot 2024-06-19 at 16 20 10

Alterative Version

Screenshot 2024-06-19 at 16 20 30

Benchmark

This refactor did turn out to be slightly slower than the existing one in a benchmark.
Screenshot 2024-06-19 at 16 32 06

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.51%. Comparing base (b75d42a) to head (c59b2b6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5082      +/-   ##
==========================================
- Coverage   92.51%   92.51%   -0.01%     
==========================================
  Files         481      481              
  Lines       38722    38719       -3     
  Branches     5444     5441       -3     
==========================================
- Hits        35822    35819       -3     
  Misses       2845     2845              
  Partials       55       55              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

This is great. Thank you for the solid troubleshooting.

streams/early_zip_readable_streams.ts Outdated Show resolved Hide resolved
streams/early_zip_readable_streams.ts Outdated Show resolved Hide resolved
streams/early_zip_readable_streams_test.ts Show resolved Hide resolved
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@iuioiua iuioiua changed the title refactor(streams): earlyZipReadableStreams fix(streams): prevent earlyZipReadableStreams() from possibly using excessive memory Jun 20, 2024
Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

I made a final tweak to the cancel reason. Thank you very much, @BlackAsLight. Great work.

@iuioiua iuioiua enabled auto-merge (squash) June 20, 2024 05:35
@iuioiua iuioiua merged commit 5bcbb8f into denoland:main Jun 20, 2024
15 checks passed
@BlackAsLight BlackAsLight deleted the early_zip_readable_stream_reactor_test branch June 20, 2024 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants