Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

rayw000
Copy link
Contributor

@rayw000 rayw000 commented Sep 28, 2021 •
edited
Loading

What changed

  1. 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.
  2. Update document.

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

Maybe fix: #40059

@nodejs-github-bot 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
@rayw000 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 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 rayw000 mentioned this pull request Sep 29, 2021
Closed
tniessen
tniessen reviewed Oct 4, 2021
View reviewed changes
doc/api/v8.md Outdated Show resolved Hide resolved
doc/api/v8.md Outdated Show resolved Hide resolved
@rayw000 rayw000 requested a review from tniessen October 5, 2021 01:11
@Mesteery 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@Mesteery Mesteery added aix Issues and PRs related to the AIX platform. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed aix Issues and PRs related to the AIX platform. labels Oct 13, 2021
@rayw000
Copy link
Contributor Author

rayw000 commented Oct 13, 2021

Hi @Mesteery

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?

@Mesteery
Copy link
Contributor

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Oct 14, 2021
Open
22 tasks
@rayw000 rayw000 mentioned this pull request Oct 14, 2021
Closed
This was referenced Oct 15, 2021
Open
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Oct 23, 2021
@rayw000 @targos
src: throw error instead of assertion
7b49e29 
PR-URL: #40243
Fixes: #40059
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Oct 23, 2021
@rayw000 @targos
doc: describe buffer limit of v8.serialize
14f6c67 
PR-URL: #40243
Fixes: #40059
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@targos
Copy link
Member

targos commented Oct 23, 2021

Landed in c83f47f...14f6c67

@targos targos closed this Oct 23, 2021
targos pushed a commit that referenced this pull request Oct 23, 2021
@rayw000 @targos
src: throw error instead of assertion
396342e 
PR-URL: #40243
Fixes: #40059
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Oct 23, 2021
@rayw000 @targos
doc: describe buffer limit of v8.serialize
305c022 
PR-URL: #40243
Fixes: #40059
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@rayw000 rayw000 deleted the throw-error-instead-of-assert-when-buffer-too-large branch October 23, 2021 07:19
@targos targos mentioned this pull request Nov 8, 2021
Merged
BethGriggs pushed a commit that referenced this pull request Nov 24, 2021
@rayw000 @BethGriggs
src: throw error instead of assertion
789fef1 
PR-URL: #40243
Fixes: #40059
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
BethGriggs pushed a commit that referenced this pull request Nov 24, 2021
@rayw000 @BethGriggs
doc: describe buffer limit of v8.serialize
579f01c 
PR-URL: #40243
Fixes: #40059
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Nov 26, 2021
Merged
1 task
kvakil added a commit to kvakil/node that referenced this pull request Jul 22, 2022
@kvakil
buffer: do not leak memory if buffer is too big
d13bc57 
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.

Included is a test which provokes this behavior using `v8.serialize`.
Technically the behavior is allocator-dependent, but I suspect any
reasonable allocator will choose to free (or at the very least, reuse)
the 1 GiB memory.

Refs: nodejs#40243
@kvakil kvakil mentioned this pull request Jul 22, 2022
Merged
kvakil added a commit to kvakil/node that referenced this pull request Jul 22, 2022
@kvakil
buffer: do not leak memory if buffer is too big
4c2d9a6 
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.

Included is a test which provokes this behavior using `v8.serialize`.
Technically the behavior is allocator-dependent, but I suspect any
reasonable allocator will choose to free (or at the very least, reuse)
the 1 GiB memory.

Refs: nodejs#40243
kvakil added a commit to kvakil/node that referenced this pull request Jul 23, 2022
@kvakil
buffer: do not leak memory if buffer is too big
c949bf1 
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
nodejs-github-bot pushed a commit that referenced this pull request Jul 24, 2022
@kvakil
buffer: do not leak memory if buffer is too big
e8e92ec 
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 26, 2022
@kvakil @danielleadams
buffer: do not leak memory if buffer is too big
0783ddf 
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@kvakil @targos
buffer: do not leak memory if buffer is too big
0f3f2c9 
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@kvakil @targos
buffer: do not leak memory if buffer is too big
105453b 
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Aug 1, 2022
@kvakil @targos
buffer: do not leak memory if buffer is too big
60d032b 
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
@kvakil @Fyko
buffer: do not leak memory if buffer is too big
020f246 
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

PR-URL: nodejs#43938
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@kvakil @guangwong
buffer: do not leak memory if buffer is too big
b3e5d52 
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 <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@tniessen tniessen Awaiting requested review from tniessen

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document (or fix?) v8.deserialize' 2gb limitation for input buffer
6 participants
@rayw000 @nodejs-github-bot @Mesteery @targos @jasnell @tniessen