Skip to content

zlib: prevent oversized ArrayBuffer retention in one-shot methods - #65647

Open
Abhirup0 wants to merge 1 commit into
nodejs:mainfrom
Abhirup0:fix/zlib-one-shot-memory-leak
Open

zlib: prevent oversized ArrayBuffer retention in one-shot methods#65647
Abhirup0 wants to merge 1 commit into
nodejs:mainfrom
Abhirup0:fix/zlib-one-shot-memory-leak

Conversation

@Abhirup0

Copy link
Copy Markdown

Fixes: #65600

When one-shot convenience methods (zlib.gzip(), zlib.deflate(),
zlib.brotliCompress(), zlib.zstdCompress(), etc.) complete with a single
output chunk, zlibBufferOnEnd and processChunkSync returned bufs[0],
which was a sub-slice of the 16 KB default _outBuffer. For small outputs,
this retained the full 16,384-byte backing ArrayBuffer.

Because this.close() synchronously frees the native C++ context and
decrements the external memory accounter back to 0, V8 does not observe
external memory pressure or significant JS heap growth. In long-running
processes with comfortable heaps, dead ArrayBuffer allocations accumulated
until the process ran out of memory.

This change:

  1. Trims single-chunk one-shot results to exact size with Buffer.from() when
    the chunk is smaller than its backing ArrayBuffer.
  2. Releases internal references to _outBuffer, buffers, cb, and
    _writeState on stream completion, error, and close.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies. labels Aug 29, 2026
@MikeMcC399

Copy link
Copy Markdown
Contributor

This PR fails many tests.

Please make sure you have read and understood the following documents:

@codecov

codecov Bot commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.06%. Comparing base (7b0de5e) to head (bb8cd2a).
⚠️ Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
lib/zlib.js 66.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65647   +/-   ##
=======================================
  Coverage   90.05%   90.06%           
=======================================
  Files         754      754           
  Lines      255722   255748   +26     
  Branches    48314    48331   +17     
=======================================
+ Hits       230281   230327   +46     
+ Misses      16555    16538   -17     
+ Partials     8886     8883    -3     
Files with missing lines Coverage Δ
lib/zlib.js 97.97% <66.66%> (-0.17%) ⬇️

... and 37 files with indirect coverage changes

🚀 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.

@Abhirup0
Abhirup0 force-pushed the fix/zlib-one-shot-memory-leak branch from d9ac576 to afc0e7a Compare August 30, 2026 07:51
@Abhirup0

Copy link
Copy Markdown
Author

Updated the commit to follow commit message line wrapping and DCO sign-off guidelines, updated the test suite to use common.mustSucceed(), and narrowed the implementation cleanly to the single-chunk buffer trimming in zlibBufferOnEnd and processChunkSync. CI checks should now be green.

@Abhirup0
Abhirup0 force-pushed the fix/zlib-one-shot-memory-leak branch from afc0e7a to 6739f06 Compare August 30, 2026 08:19
When one-shot convenience methods (zlib.gzip(), zlib.deflate(),
zlib.brotliCompress(), zlib.zstdCompress(), etc.) complete with a
single output chunk, zlibBufferOnEnd and processChunkSync returned
bufs[0], which was a sub-slice of the 16 KB default _outBuffer. For
small outputs, this retained the full 16,384-byte backing ArrayBuffer.

Because this.close() synchronously frees the native C++ context and
decrements the external memory accounter back to 0, V8 does not observe
external memory pressure or significant JS heap growth. In long-running
processes with comfortable heaps, dead ArrayBuffer allocations
accumulated until the process ran out of memory.

This change trims single-chunk one-shot results to exact size with
Buffer.from() when the chunk is smaller than its backing ArrayBuffer.

Fixes: nodejs#65600
Signed-off-by: Abhirup Karmakar <abhirup.karmakar00@gmail.com>
@Abhirup0
Abhirup0 force-pushed the fix/zlib-one-shot-memory-leak branch from 6739f06 to bb8cd2a Compare August 30, 2026 09:40
@MikeMcC399

Copy link
Copy Markdown
Contributor

@Abhirup0

You have forced-push several changes in the meantime, and CI continues to fail.

Before you submit changes, you should test these locally. See https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-6-test

@Abhirup0

Abhirup0 commented Aug 30, 2026

Copy link
Copy Markdown
Author

Apologies for the noise and multiple CI runs, @MikeMcC399.

The issue with the earlier test was an assumption about Buffer.poolSize: Buffer.from(singleChunk) correctly copies small single-chunk results into Node's shared Buffer pool to release the stream's unpooled _outBuffer, but on current main, Buffer.poolSize was increased to 64 KB (65,536 bytes). The previous test had hardcoded < 16384, which caused the assertion to fail against the 64 KB pool size.

In bb8cd2a, the test was updated to use chunkSize = 128 * 1024 and assert buf.buffer.byteLength < chunkSize alongside buf.buffer.byteLength <= Buffer.poolSize, properly verifying that the 128 KB unpooled allocation is released while respecting Node's shared buffer pool.

Whenever convenient, could CI please be re-run on bb8cd2a? Thank you for your patience and guidance!

@MikeMcC399

Copy link
Copy Markdown
Contributor

@Abhirup0

The GitHub Actions CI continues to fail.

Did you test this branch locally according to https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-6-test ?

Which operating system are you testing on?

Are you relying on AI assistance for this PR to make changes and to write comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

zlib: one-shot gzip()/deflate() results accumulate in arrayBuffers until OOM — GC never prompted (regression in v24.15.0)

4 participants