- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 3.2k
 
feat: fixes mem leak relating to issue-1834 #1893
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
Conversation
          Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@           Coverage Diff           @@
##           master    #1893   +/-   ##
=======================================
  Coverage   99.90%   99.90%           
=======================================
  Files           9        9           
  Lines        2060     2074   +14     
=======================================
+ Hits         2058     2072   +14     
  Misses          2        2           ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
  | 
    
f809804    to
    7c93f8d      
    Compare
  
    7c93f8d    to
    958e824      
    Compare
  
    777a840    to
    8dc41ab      
    Compare
  
    | 
           @fengmk2 could you review this as you have time? 🙏 🙇  | 
    
| 
           Is it the same issue fixed by #1889?  | 
    
60cb0ce    to
    4670ea1      
    Compare
  
    | 
           @fengmk2 updated. 
  | 
    
4670ea1    to
    5af8a64      
    Compare
  
    | 
           @jonathanong Do you want to take another look?  | 
    
| 
           I will merge this fix as a better stream handler and release it as a minor version. 
  | 
    
| 
           @yowainwright Thank you for your excellent contribution!  | 
    
There was a problem hiding this 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 memory leaks related to stream handling in Koa by replacing the .pipe() method with Stream.pipeline() and improving stream cleanup. The changes ensure proper stream destruction and error handling to prevent resource leaks when streams are replaced or responses are aborted.
Key changes:
- Replaced 
.pipe()withStream.pipeline()for automatic stream cleanup and error handling - Added explicit cleanup of original streams when the response body is set to null
 - Removed manual error listeners on streams since pipeline handles errors automatically
 
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description | 
|---|---|
| lib/response.js | Adds cleanup logic to destroy original streams when body is nullified, removes manual error listener attachment | 
| lib/application.js | Refactors stream handling to use Stream.pipeline() instead of .pipe() for proper cleanup and error propagation | 
| tests/response/body.test.js | Updates test expectations to reflect removal of manual error listeners on streams | 
| tests/application/respond.test.js | Adds comprehensive test coverage for ReadableStream, Response objects, and pipeline error handling scenarios | 
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
5af8a64    to
    954a474      
    Compare
  
    | 
           @yowainwright If no one gets feedback this week, I'll release this change on Sunday.  | 
    
| if (isStream(val)) { | ||
| onFinish(this.res, destroy.bind(null, val)) | ||
| if (original !== val) { | ||
| val.once('error', err => this.ctx.onerror(err)) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yowainwright I consecutively set 2 streams that will cause exceptions, deleting here is problematic.
ctx.body = fs.createReadStream('does not exist');
ctx.body = fs.createReadStream('does not exist');will throw unhandled error: Error: ENOENT: no such file or directory, open 'does not exist'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fengmk2 update below. 🙇
**Adds stream clean-up** in addition to nullish value clean-up. - update from @fengmk2's identified issue [here](#1893 (comment)). - #1893 (comment) - Stackblitz display issue + fix [here](https://stackblitz.com/~/github.com/yowainwright/koa-patch-ISSUE-1834_discussion_r2455395874?file=patch/koa/lib/response.js). - https://stackblitz.com/~/github.com/yowainwright/koa-patch-ISSUE-1834_discussion_r2455395874?file=patch/koa/lib/response.js
| 
           v3.1.0 released https://github.com/koajs/koa/releases/tag/v3.1.0  | 
    
Description
Checklist
Checklist for 1834 (before re-pinging team)