FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

buffer: fix assertion error in WeakCallback by indutny · Pull Request #3329 · nodejs/node · GitHub

/ node Public

buffer: fix assertion error in WeakCallback - #3329

Closed
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:fix/buffer-free-callback-regr
ConversationCommits (2)ChecksFiles changed
Closed

buffer: fix assertion error in WeakCallback#3329
indutny wants to merge 2 commits into
nodejs:masterfrom
indutny:fix/buffer-free-callback-regr

Conversation

indutny commented Oct 12, 2015

Copy link
Copy Markdown
Member

CallbackInfo is now bound to ArrayBuffer instance, not Uint8Array,
therefore SPREAD_ARG will abort with:

Assertion failed: ((object)->IsUint8Array())

Make changes necessary to migrate it to ArrayBuffer.

See: #3080 (comment)

R=@trevnorris

buffer: fix assertion error in WeakCallback
9d4624a 
`CallbackInfo` is now bound to `ArrayBuffer` instance, not `Uint8Array`,
therefore `SPREAD_ARG` will abort with:

    Assertion failed: ((object)->IsUint8Array())

Make changes necessary to migrate it to `ArrayBuffer`.

See: nodejs#3080 (comment)
indutny mentioned this pull request Oct 12, 2015
Closed

indutny commented Oct 12, 2015

Copy link
Copy Markdown
Member Author

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

trevnorris reviewed Oct 12, 2015
View reviewed changes
Comment thread src/node_buffer.cc Outdated

trevnorris Oct 12, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@TooTallNate has mentioned how some use a zero-length buffer to keep track of things in their modules. A zero-length buffer would be nullptr. Instead should be like:

if (obj_c.Length() > 0) CHECK_NE(obj_data, nullptr);

trevnorris Oct 12, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

might want to add a test for this case as well.

trevnorris commented Oct 12, 2015

Copy link
Copy Markdown
Contributor

Left 2 comments. Other than that, LGTM

mikemorris mentioned this pull request Oct 12, 2015
Closed

indutny commented Oct 12, 2015

Copy link
Copy Markdown
Member Author

@trevnorris fixed!

trevnorris commented Oct 12, 2015

Copy link
Copy Markdown
Contributor

LGTM

indutny commented Oct 12, 2015

Copy link
Copy Markdown
Member Author

@mikemorris may I ask you to give a try to this patch? Does it fix problem for you?

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Oct 12, 2015

mikemorris commented Oct 12, 2015

Copy link
Copy Markdown

@indutny Looks like that fixed it, thanks for the quick response @indutny!

indutny commented Oct 12, 2015

Copy link
Copy Markdown
Member Author

Great, thank you for confirming this! cc @jasnell

indutny added a commit that referenced this pull request Oct 13, 2015
buffer: fix assertion error in WeakCallback
931118c 
`CallbackInfo` is now bound to `ArrayBuffer` instance, not `Uint8Array`,
therefore `SPREAD_ARG` will abort with:

    Assertion failed: ((object)->IsUint8Array())

Make changes necessary to migrate it to `ArrayBuffer`.

See: #3080 (comment)

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: #3329

jasnell commented Oct 13, 2015

Copy link
Copy Markdown
Member

Landed in 931118c

indutny added a commit that referenced this pull request Oct 13, 2015
buffer: fix assertion error in WeakCallback
b3cbd13 
`CallbackInfo` is now bound to `ArrayBuffer` instance, not `Uint8Array`,
therefore `SPREAD_ARG` will abort with:

    Assertion failed: ((object)->IsUint8Array())

Make changes necessary to migrate it to `ArrayBuffer`.

See: #3080 (comment)

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: #3329

jasnell commented Oct 13, 2015

Copy link
Copy Markdown
Member

Landed in v4.x in b3cbd13

jasnell closed this Oct 13, 2015
jasnell added a commit that referenced this pull request Oct 13, 2015
2015-10-13, Version 4.2.1 'Argon' (LTS) Release
2f39185 
* Includes fixes for two regressions
  - Assertion error in WeakCallback  - see [#3329](#3329)
  - Undefined timeout regression - see [#3331](#3331)
jasnell mentioned this pull request Oct 13, 2015
Closed
kkoopa mentioned this pull request Oct 13, 2015
Closed
jasnell added a commit that referenced this pull request Oct 13, 2015
2015-10-13, Version 4.2.1 'Argon' (LTS) Release
9ddb251 
* Includes fixes for two regressions
  - Assertion error in WeakCallback  - see [#3329](#3329)
  - Undefined timeout regression - see [#3331](#3331)
jasnell added a commit that referenced this pull request Oct 13, 2015
2015-10-13, Version 4.2.1 'Argon' (LTS) Release
659291c 
* Includes fixes for two regressions
  - Assertion error in WeakCallback  - see [#3329](#3329)
  - Undefined timeout regression - see [#3331](#3331)

* Document an additional known issue with pipelined requests
  - See: #3332 and #3342
jasnell added a commit that referenced this pull request Oct 13, 2015
2015-10-13, Version 4.2.1 'Argon' (LTS) Release
b7eff48 
* Includes fixes for two regressions
  - Assertion error in WeakCallback  - see [#3329](#3329)
  - Undefined timeout regression - see [#3331](#3331)

* Document an additional known issue with pipelined requests
  - See: #3332 and #3342
jasnell added a commit that referenced this pull request Oct 13, 2015
2015-10-13, Version 4.2.1 'Argon' (LTS) Release
0a7076b 
* Includes fixes for two regressions
  - Assertion error in WeakCallback  - see [#3329](#3329)
  - Undefined timeout regression - see [#3331](#3331)

* Document an additional known issue with pipelined requests
  - See: #3332 and #3342
Starefossen mentioned this pull request Oct 13, 2015
Merged
Trott reviewed Jan 4, 2019
View reviewed changes
Comment thread test/addons/buffer-free-callback/test.js
slice = null;
gc();
gc();
gc();

Trott Jan 4, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's unclear to me why calling gc() three times is required here. @indutny Sorry to pull this up from more than 3 years ago, but do you remember why?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL