buffer,doc: Throw error instead of assert when buffer too large by rayw000 · Pull Request #40243 · nodejs/node · GitHub

What changed

Throw error instead of assertion when buffer is too large. Users can somehow reach the assertion, i.e. const buffer = v8.serialize(<huge object>). In this case, an exception would be better than assertion.
Update document.

Additional: https://chromium-review.googlesource.com/c/v8/v8/+/3170411

Maybe fix: #40059

CI: https://ci.nodejs.org/job/node-test-pull-request/40262/

I'm not sure that I fully understand the error report CI gives. Some of them are timeout and others are no test.
Perhaps a retrying could fix it?

I think these are flaky tests, they are not related to this PR.

CI: https://ci.nodejs.org/job/node-test-pull-request/40283/

PR-URL: #40243 Fixes: #40059 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Landed in c83f47f...14f6c67

PR-URL: #40243 Fixes: #40059 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

A recent pull request changed this method to throw when the buffer was too big, but this meant that the `free` finalizer would never get called, leading to a memory leak. A previous version of this diff included a test provoking this behavior with `v8.serialize`, but it unfortunately kept triggering the OOM killer, so it was removed. Refs: nodejs#40243

A recent pull request changed this method to throw when the buffer was too big, but this meant that the `free` finalizer would never get called, leading to a memory leak. A previous version of this diff included a test provoking this behavior with `v8.serialize`, but it unfortunately kept triggering the OOM killer, so it was removed. Refs: #40243 PR-URL: #43938 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

A recent pull request changed this method to throw when the buffer was too big, but this meant that the `free` finalizer would never get called, leading to a memory leak. A previous version of this diff included a test provoking this behavior with `v8.serialize`, but it unfortunately kept triggering the OOM killer, so it was removed. Refs: nodejs/node#40243 PR-URL: nodejs/node#43938 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 28, 2021

src: throw error instead of assertion

1534112

rayw000 changed the title Throw error instead of assert when buffer too large buffer+doc: Throw error instead of assert when buffer too large Sep 29, 2021

rayw000 changed the title buffer+doc: Throw error instead of assert when buffer too large buffer,doc: Throw error instead of assert when buffer too large Sep 29, 2021

rayw000 mentioned this pull request Sep 29, 2021

Document (or fix?) v8.deserialize' 2gb limitation for input buffer #40059

Closed

jasnell approved these changes Sep 30, 2021

View reviewed changes

tniessen reviewed Oct 4, 2021

View reviewed changes

Comment thread doc/api/v8.md Outdated

Comment thread doc/api/v8.md Outdated

doc: describe buffer limit of v8.serialize

2cb3651

rayw000 requested a review from tniessen October 5, 2021 01:11

Mesteery added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Oct 12, 2021

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021

Mesteery added aix Issues and PRs related to the AIX platform. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. and removed aix Issues and PRs related to the AIX platform. labels Oct 13, 2021

github-actions Bot mentioned this pull request Oct 14, 2021

CI Reliability 2021-10-14 nodejs/reliability#86

Open

22 tasks

rayw000 mentioned this pull request Oct 14, 2021

deps: V8: cherry-pick 422dc378a1da #40450

Closed

targos approved these changes Oct 23, 2021

View reviewed changes

targos pushed a commit that referenced this pull request Oct 23, 2021

src: throw error instead of assertion

7b49e29

PR-URL: #40243 Fixes: #40059 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos pushed a commit that referenced this pull request Oct 23, 2021

doc: describe buffer limit of v8.serialize

14f6c67

PR-URL: #40243 Fixes: #40059 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos closed this Oct 23, 2021

targos pushed a commit that referenced this pull request Oct 23, 2021

src: throw error instead of assertion

396342e

PR-URL: #40243 Fixes: #40059 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

targos pushed a commit that referenced this pull request Oct 23, 2021

doc: describe buffer limit of v8.serialize

305c022

PR-URL: #40243 Fixes: #40059 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

rayw000 deleted the throw-error-instead-of-assert-when-buffer-too-large branch October 23, 2021 07:19

targos mentioned this pull request Nov 8, 2021

v17.1.0 release proposal #40758

Merged

BethGriggs pushed a commit that referenced this pull request Nov 24, 2021

src: throw error instead of assertion

789fef1

PR-URL: #40243 Fixes: #40059 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

BethGriggs pushed a commit that referenced this pull request Nov 24, 2021

doc: describe buffer limit of v8.serialize

579f01c

PR-URL: #40243 Fixes: #40059 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

BethGriggs mentioned this pull request Nov 26, 2021

v16.13.1 proposal #40974

Merged

1 task

kvakil mentioned this pull request Jul 22, 2022

buffer: do not leak memory if buffer is too big #43938

Merged

buffer,doc: Throw error instead of assert when buffer too large by rayw000 · Pull Request #40243 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer,doc: Throw error instead of assert when buffer too large - #40243

buffer,doc: Throw error instead of assert when buffer too large#40243
rayw000 wants to merge 2 commits into
nodejs:masterfrom
rayw000:throw-error-instead-of-assert-when-buffer-too-large

rayw000 commented Sep 28, 2021 •
edited
Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 12, 2021

Uh oh!

rayw000 commented Oct 13, 2021

Uh oh!

Mesteery commented Oct 13, 2021

Uh oh!

nodejs-github-bot commented Oct 13, 2021

Uh oh!

targos commented Oct 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

buffer,doc: Throw error instead of assert when buffer too large by rayw000 · Pull Request #40243 · nodejs/node · GitHub

Uh oh!

Uh oh!

buffer,doc: Throw error instead of assert when buffer too large - #40243

buffer,doc: Throw error instead of assert when buffer too large#40243rayw000 wants to merge 2 commits intonodejs:masternodejs/node:masterfrom rayw000:throw-error-instead-of-assert-when-buffer-too-largeCopy head branch name to clipboard

Conversation

rayw000 commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 12, 2021

Uh oh!

rayw000 commented Oct 13, 2021

Uh oh!

Mesteery commented Oct 13, 2021

Uh oh!

nodejs-github-bot commented Oct 13, 2021

Uh oh!

targos commented Oct 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

buffer,doc: Throw error instead of assert when buffer too large#40243
rayw000 wants to merge 2 commits into
nodejs:masterfrom
rayw000:throw-error-instead-of-assert-when-buffer-too-large

rayw000 commented Sep 28, 2021 •
edited
Loading