buffer: allow .write() offset to be at buffer end #8154
Closed
Conversation
This file contains 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
Do not throw if the offset passed to `buf.write()` points to the end of the buffer. Fixes: nodejs#8127
|
Given that this removes a throw, we need to decide if it's semver-major or not. I think it's likely safe as a patch. The change itself LGTM if CI is green |
|
This came up as an unintended regression in v6.4.0, so yeah, definitely +1 for considering this a semver-patch one. |
|
Windows fanned test builds were having Jenkins problems. Re-running CI: https://ci.nodejs.org/job/node-test-pull-request/3721/ |
| @@ -358,6 +358,9 @@ writeTest.write('e', 3, 'ascii'); | |||
| writeTest.write('j', 4, 'ascii'); | |||
| assert.equal(writeTest.toString(), 'nodejs'); | |||
|
|
|||
| // Does not throw (see https://github.com/nodejs/node/issues/8127). | |||
| Buffer.alloc(1).write('', 1, 0); | |||
There was a problem hiding this comment.
Could you put this in an assert.doesNotThrow()?
|
LGTM |
|
This is definitely a semver-patch, because it reverts a breaking change that was accidentally introduce in a semver-minor version. |
|
Still LGTM. Just to be unnecessarily-extra-cautious, should probably do one more CI run following the new commit but I'm good with it landing either way. |
|
CI: https://ci.nodejs.org/job/node-test-commit/4646/ … because running again on the latest changes is probably just the right amount of cautious ;) |
|
LGTM |
|
Landed in 3242b27! |
4 tasks
evanlucas
added a commit
that referenced
this pull request
Aug 24, 2016
Notable changes: * **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154 * **inspector**: * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021 * add support for uncaught exception (Aleksei Koziatinskii) #8043 * **meta**: add @joshgav to collaborators (Josh Gavant) #8146 * **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145 * ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143
evanlucas
added a commit
that referenced
this pull request
Aug 26, 2016
Notable changes: * **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154 * **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054 * **inspector**: * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021 * add support for uncaught exception (Aleksei Koziatinskii) #8043 * **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145 * ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143 PR-URL: #8253
evanlucas
added a commit
that referenced
this pull request
Aug 26, 2016
Notable changes: * **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154 * **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054 * **inspector**: * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021 * add support for uncaught exception (Aleksei Koziatinskii) #8043 * **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145 * ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143 PR-URL: #8253
arthurschreiber
added a commit
to tediousjs/tedious
that referenced
this pull request
Aug 26, 2016
Node.js 6.4.0 was shipped with a bug in it's Buffer implementation that causes the tedious test cases to fail. See #431, nodejs/node#8127 and nodejs/node#8154.
evanlucas
added a commit
that referenced
this pull request
Aug 26, 2016
Notable changes: * **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154 * **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054 * **inspector**: * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021 * add support for uncaught exception (Aleksei Koziatinskii) #8043 * **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145 * ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143 PR-URL: #8253
tvrprasad
pushed a commit
to tvrprasad/tedious
that referenced
this pull request
Sep 11, 2016
Node.js 6.4.0 was shipped with a bug in it's Buffer implementation that causes the tedious test cases to fail. See tediousjs#431, nodejs/node#8127 and nodejs/node#8154.
|
@addaleax should this be backported? |
|
It depends on/fixes a problem with #7494, so, probably not… although I can’t quite see why that one landed as |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
buffer
Description of change
Do not throw if the offset passed to
buf.write()points to the end of the buffer.Fixes: #8127
CI: https://ci.nodejs.org/job/node-test-commit/4633/