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.

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

buffer: expose underlying buffer object always #8311

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@thefourtheye
Contributor

thefourtheye commented Aug 28, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

If the Buffer object's length is zero, or equal to the underlying
buffer object's length, parent property returns undefined.

> new Buffer(0).parent
undefined
> new Buffer(Buffer.poolSize).parent
undefined

This patch makes the buffer objects to consistently expose the buffer
object via the parent property, always.

Fixes: #8266


cc @nodejs/buffer

buffer: expose underlying buffer object always
If the Buffer object's length is zero, or equal to the underlying
buffer object's length, `parent` property returns `undefined`.

    > new Buffer(0).parent
    undefined
    > new Buffer(Buffer.poolSize).parent
    undefined

This patch makes the buffer objects to consistently expose the buffer
object via the `parent` property, always.

Fixes: #8266

@thefourtheye thefourtheye added the buffer label Aug 28, 2016

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Aug 28, 2016

LGTM. Though can we also move to deprecate .parent?

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 29, 2016

LGTM … what do you think on the semver-ness of this? It doesn’t seem like something that would reasonably break anybody’s code, but it does involve having to change the existing tests.

CI: https://ci.nodejs.org/job/node-test-commit/4812/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/373/

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 29, 2016

CI looks okay, for CITGM… @thealphanerd are those known failures? (Also… so many DeprecationWarnings…)

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 29, 2016

+1 to a runtime deprecation on .parent, but that can be done as a separate semver-major PR. This one should be safe as a semver-minor.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 29, 2016

LGTM

@addaleax addaleax referenced this pull request Aug 29, 2016

Closed

meta: deprecation policy #7964

2 of 2 tasks complete

@thefourtheye thefourtheye referenced this pull request Aug 30, 2016

Closed

buffer: deprecate parent property #8332

2 of 2 tasks complete
@addaleax

This comment has been minimized.

Member

addaleax commented Sep 5, 2016

@thefourtheye This is good to go, right? :)

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Sep 6, 2016

@addaleax I would say so, as I have seen the vinyl-fs failure in few other runs as well. I thought it would be better if @thealphanerd or @phated could give GO/NO-GO here.

@phated

This comment has been minimized.

Contributor

phated commented Sep 6, 2016

Pretty sure these are permission problems and the citgm needs to be fixed.

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 7, 2016

+1 to getting this landed :-)

@addaleax

This comment has been minimized.

Member

addaleax commented Sep 8, 2016

Landed in c21458a

@addaleax addaleax closed this Sep 8, 2016

addaleax added a commit that referenced this pull request Sep 8, 2016

buffer: expose underlying buffer object always
If the Buffer object's length is zero, or equal to the underlying
buffer object's length, `parent` property returns `undefined`.

    > new Buffer(0).parent
    undefined
    > new Buffer(Buffer.poolSize).parent
    undefined

This patch makes the buffer objects to consistently expose the buffer
object via the `parent` property, always.

Fixes: #8266
PR-URL: #8311
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

@thefourtheye thefourtheye deleted the thefourtheye:buffer-parent-property branch Sep 10, 2016

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Sep 13, 2016

Getting this trying to land on v6-staging:

/usr/bin/python tools/test.py --mode=release -J \
        addons doctool known_issues message pseudo-tty parallel sequential
=== release test-buffer ===                                                    
Path: parallel/test-buffer
b.length == 1024
c.length == 512
copied 512 bytes from b into c
copied 512 bytes from c into b w/o sourceEnd
copied 512 bytes from c into b w/o sourceStart
copied 512 bytes from b into c w/o targetStart
copied 256 bytes from end of b into beginning of c
copied 512 bytes from b trying to overrun c
copied 768 bytes from b into b
copied 512 bytes from b into c
test copy at negative sourceEnd
bytes written to buffer: 11
Create hex string from buffer
Create buffer from hex string
Try to slice off the end of the buffer
<Buffer 81 a3 66 6f 6f a3 62 61 72>
<Buffer 6f a3 62 61 72>
5
<Buffer 81 a3 66 6f 6f a3 62 61 72>
9
<Buffer 81 a3 66 6f>
4
<Buffer 81 a3 66 6f 6f a3 62 61 72>
9
<Buffer a3 66 6f>
3
<Buffer 66 6f>
2
<Buffer ff 61 62 ff>
<Buffer ff 61 62 63>
<Buffer ff 61 62 ff>
<Buffer ff ab cd ff>
<Buffer 61 00 ff ff>
<Buffer 61 00 ff ff>
<Buffer 61 00 ff ff>
<Buffer 61 00 ff ff>
uber: 'über'
f.length: 4     (should be 4)
f.length: 8     (should be 8)
f.length: 12     (should be 12)
bytes written to buffer: 4     (should be 4)
f.length: 8     (should be 8)
f.length: 12     (should be 12)
bytes written to buffer: 4     (should be 4)
f.length: 8     (should be 8)
f.length: 12     (should be 12)
bytes written to buffer: 4     (should be 4)
f.length: 8     (should be 8)
f.length: 12     (should be 12)
bytes written to buffer: 4     (should be 4)

assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: ArrayBuffer { byteLength: 1 } == undefined
    at Object.<anonymous> (/Users/Jeremiah/Documents/node/test/parallel/test-buffer.js:1466:8)
    at Module._compile (module.js:556:32)
    at Object.Module._extensions..js (module.js:565:10)
    at Module.load (module.js:473:32)
    at tryModuleLoad (module.js:432:12)
    at Function.Module._load (module.js:424:3)
    at Module.runMain (module.js:590:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3
Command: out/Release/node /Users/Jeremiah/Documents/node/test/parallel/test-buffer.js
[01:13|% 100|+ 1229|-   1]: Done                                               
make: *** [test] Error 1
c8099732b6bf6d9bbe320f62df71614d26b77126 is the first bad commit
commit c8099732b6bf6d9bbe320f62df71614d26b77126
Author: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Date:   Sun Aug 28 21:21:23 2016 +0530

    buffer: expose underlying buffer object always

    If the Buffer object's length is zero, or equal to the underlying
    buffer object's length, `parent` property returns `undefined`.

        > new Buffer(0).parent
        undefined
        > new Buffer(Buffer.poolSize).parent
        undefined

    This patch makes the buffer objects to consistently expose the buffer
    object via the `parent` property, always.

    Fixes: https://github.com/nodejs/node/issues/8266
    PR-URL: https://github.com/nodejs/node/pull/8311
    Reviewed-By: Trevor Norris <trev.norris@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: James M Snell <jasnell@gmail.com>

:040000 040000 e81f69514f6b228dfd5b192a349f6ce1b3cc319a 807e99d3633abdf274fc5ffc92c208013daca806 M  lib
:040000 040000 da006be5fd497c7872077ef3d43878346495b7c9 52aec362d21e176cfe565bbc5b1932febfe8d6b4 M  test
bisect run success

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment