feat: fixes mem leak relating to issue-1834#1893
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.
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.
@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'
**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 |
**Adds stream clean-up** in addition to nullish value clean-up. - update from @fengmk2's identified issue [here](koajs/koa#1893 (comment)). - koajs/koa#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
Description
Checklist
Checklist for 1834 (before re-pinging team)