Skip to content

Conversation

@yowainwright
Copy link
Member

@yowainwright yowainwright commented Oct 23, 2025

Discription

Adds stream clean-up in addition to nullish value clean-up.

Checklist

  • I have ensured my pull request is not behind the main or master branch of the original repository.
  • I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • I have written a commit message that passes commitlint linting.
  • I have ensured that my code changes pass linting tests.
  • I have ensured that my code changes pass unit tests.
  • I have described my pull request and the reasons for code changes along with context if necessary.

@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (49a3207) to head (995426e).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1910   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files           9        9           
  Lines        2074     2081    +7     
=======================================
+ Hits         2072     2079    +7     
  Misses          2        2           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yowainwright yowainwright requested a review from Copilot October 23, 2025 17:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a resource leak by ensuring that when a response body stream is replaced with another stream or null value, the original stream is properly destroyed to prevent resource leaks. The fix adds cleanup logic that checks if the original value is a stream and destroys it when it's being replaced, while also preventing unhandled error events during cleanup.

Key Changes:

  • Adds stream cleanup logic when a response body stream is replaced
  • Includes error handling to prevent unhandled exceptions during stream destruction
  • Adds comprehensive test coverage for various stream replacement scenarios

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/response.js Implements stream cleanup logic that destroys the original stream when replaced, with error suppression to prevent unhandled exceptions
tests/response/body.test.js Adds five test cases covering stream replacement scenarios including cleanup verification and error handling

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@yowainwright yowainwright force-pushed the ISSUE-1834_discussion_r2455395874 branch from 028be8c to 995426e Compare October 23, 2025 17:15
@yowainwright yowainwright requested a review from Copilot October 23, 2025 17:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@fengmk2 fengmk2 merged commit 27df3b6 into master Oct 24, 2025
6 checks passed
@fengmk2 fengmk2 deleted the ISSUE-1834_discussion_r2455395874 branch October 24, 2025 05:51
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.

3 participants