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.

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: hard-deprecate SlowBuffer #8182

Closed
wants to merge 1 commit into from

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Aug 19, 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

This emits a warning first time SlowBuffer instance is created and instructs the users to switch to Buffer.alloc() or Buffer.allocUnsafeSlow(), both of which are not pooled.

Refs: #8169
Refs: #7152

Not sure if we should do this in v7 or v8. It looks like SlowBuffer is used only by a small number of modules (I found 416, only 330 of which — outside of tests, and some of those are false positives). Module authors could use safe-buffer to polyfill for older Node.js versions, though.

Labelling as in progress because this lacks tests.

/cc @nodejs/ctc, @seishun

This emits a warning first time SlowBuffer instance is created and
instructs the users to switch to Buffer.alloc() or
Buffer.allocUnsafeSlow(), both of which are not pooled.

Refs: nodejs#8169
Refs: nodejs#7152
@ChALkeR ChALkeR added wip Issues and PRs that are still a work in progress. buffer Issues and PRs related to the buffer subsystem. discuss Issues opened for discussions and feedbacks. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 19, 2016
@seishun
Copy link
Contributor

seishun commented Aug 19, 2016

If we aren't hard-deprecating Buffer constructor in v7.0 due to v0.12 support, then the same reasoning should apply here. I don't think the number of users should be the deciding factor.

@ChALkeR
Copy link
Member Author

ChALkeR commented Aug 19, 2016

@seishun That is true, but the difference here is that SlowBuffer was documented as being for internal use in v0.10: https://nodejs.org/docs/latest-v0.10.x/api/buffer.html#buffer_class_slowbuffer

In v0.12 it got properly documented, though: https://nodejs.org/docs/latest-v0.12.x/api/buffer.html#buffer_class_slowbuffer

slowBufferWarned = true;
process.emitWarning(
'SlowBuffer is deprecated. ' +
'Use `Buffer.alloc()` or `Buffer.allocUnsafeSlow()` instead.'
Copy link
Member

@jasnell jasnell Aug 19, 2016

Choose a reason for hiding this comment

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

This would need a second argument set to 'DeprecationWarning'... that is:

process.emitWarning(
  'SlowBuffer is deprecated. ' +
  'Use `Buffer.alloc()` or `Buffer.allocUnsafeSlow()` instead.',
  'DeprecationWarning');

@jasnell
Copy link
Member

jasnell commented Aug 19, 2016

this would also need a test case verifying that the deprecation warning was emitted.

@trevnorris
Copy link
Contributor

Since SlowBuffer only ever accepted a number the polyfill may not be more complicated than

require('buffer').SlowBuffer = Buffer.unsafeAllocSlow;

@ChALkeR
Copy link
Member Author

ChALkeR commented Nov 4, 2016

This should probably be paired with #7152, so targeting 8.0 likewise.

@ChALkeR ChALkeR added this to the 8.0.0 milestone Nov 4, 2016
@ChALkeR ChALkeR added the security Issues and PRs related to security. label Nov 5, 2016
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017
@addaleax addaleax removed this from the 8.0.0 milestone Apr 1, 2017
@ChALkeR ChALkeR added security Issues and PRs related to security. and removed security Issues and PRs related to security. labels May 4, 2017
@ChALkeR
Copy link
Member Author

ChALkeR commented May 4, 2017

This won't happen soon, afaik.
There is probably no reason to deprecate SlowBuffer without deprecating Buffer(arg).
It could be reopened or replaced with a more up-to-date one when the time is right.

@ChALkeR ChALkeR closed this May 4, 2017
@ChALkeR ChALkeR added security Issues and PRs related to security. and removed security Issues and PRs related to security. stalled Issues and PRs that are stalled. labels May 4, 2017
@ChALkeR ChALkeR mentioned this pull request Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. discuss Issues opened for discussions and feedbacks. security Issues and PRs related to security. semver-major PRs that contain breaking changes and should be released in the next major version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants