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

constants: freeze the constants object #19813

Closed
wants to merge 1 commit into from

Conversation

bengl
Copy link
Member

@bengl bengl commented Apr 4, 2018

Constants ought to be constant. The primary goal of this commit is to
make constants exposed in require('constants') immutable, as they were
prior to node@7.0.0, and as the constants exposed on fs.constants,
crypto.constants, etc. are.

Since this is implemented by using Object.freeze, it also has the side
effect of making the entire exports of require('constants') immutable,
so no new constants can be defined on the object in userland.

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

Constants ought to be constant. The primary goal of this commit is to
make constants exposed in require('constants') immutable, as they were
prior to node@7.0.0, and as the constants exposed on fs.constants,
crypto.constants, etc. are.

Since this is implemented by using Object.freeze, it also has the side
effect of making the entire exports of require('constants') immutable,
so no new constants can be defined on the object in userland.
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 4, 2018
@bengl
Copy link
Member Author

bengl commented Apr 4, 2018

@bengl bengl requested a review from jasnell April 4, 2018 22:27
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 4, 2018
@Trott
Copy link
Member

Trott commented Apr 5, 2018

(Will need a CITGM run, of course.)

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if there are no horrible ecosystem repercussions.

@mscdex
Copy link
Contributor

mscdex commented Apr 5, 2018

Does a frozen object still cause performance issues when accessing properties?

mscdex
mscdex previously requested changes Apr 5, 2018
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

Although it seems like a nice idea, I think I am -1 on it as we don't freeze other parts of our API and I don't think the inconsistency is a good thing.

Also, I'm not sure I understand the comment about constants being frozen prior to node v7. I'm able to write to the constants object just fine prior to v7. I can also overwrite existing property values in node v6.x (likely due to the use of Object.assign() in lib/constants.js).

@bengl
Copy link
Member Author

bengl commented Apr 5, 2018

@mscdex Right, sorry, the change (to using Object.assign) was in 6.0.0 in commit dcccbfd. It first appeared in 7.0.0 but then was backported to 6.x in v6.3.0, so v6.3.0 is the first version (by semver ordering, not release date) that had that change. I can change that in the commit message if you'd like.

Prior to dcccbfd, constants in require('constants') are immutable:

$ nvm use 6.0.0
$ node -p "Object.getOwnPropertyDescriptor(require('constants'), 'R_OK')"
{ value: 4,
  writable: false,
  enumerable: true,
  configurable: false }

This is because prior to dcccbfd, require('constants') === process.binding('constants'). In the binding, NODE_DEFINE_CONSTANT is used:

NODE_DEFINE_CONSTANT(target, R_OK);

Which makes the properties immutable:

node/src/node.h

Lines 233 to 234 in 091abb3

v8::PropertyAttribute constant_attributes = \
static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete); \

So this is a return to the original behavior, prior to v6.3.0.

I used Object.freeze for simplicity. If desired, I can instead replace the Object.assign usage with something that copies by property descriptor, since I'm pretty sure the properties on process.binding('constants').* are still immutable. That way it wouldn't be a frozen object, but we'd still have that existing properties would be immutable, consistent with pre- dcccbfd behavior. I'd make the appropriate change to the test, of course.

@bengl
Copy link
Member Author

bengl commented Apr 5, 2018

@Trott Trott added this to the 10.0.0 milestone Apr 6, 2018
@bengl
Copy link
Member Author

bengl commented Apr 10, 2018

@mscdex Ping.

Note also that fs.constants, crypto.constants, etc., all also have immutable properties.

@addaleax addaleax removed this from the 10.0.0 milestone Apr 16, 2018
@addaleax
Copy link
Member

Taking this off the 10.0.0 milestone, the deadline has passed anyway … and ping @mscdex re: the above

@jasnell jasnell added this to the 11.0.0 milestone Apr 16, 2018
@BridgeAR BridgeAR dismissed mscdex’s stale review April 24, 2018 12:20

Dismissing review due to missing response. PTAL

@BridgeAR
Copy link
Member

@nodejs/tsc PTAL. I dismissed the review from @mscdex

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 28, 2018
@BridgeAR
Copy link
Member

I am going to land this in ~ 48 hours if there is no objection from anyone until then.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Member

BridgeAR commented May 7, 2018

Landed in 90e8f79 🎉

@BridgeAR BridgeAR closed this May 7, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 7, 2018
Constants ought to be constant. The primary goal of this commit is to
make constants exposed in require('constants') immutable, as they were
prior to node@7.0.0, and as the constants exposed on fs.constants,
crypto.constants, etc. are.

Since this is implemented by using Object.freeze, it also has the side
effect of making the entire exports of require('constants') immutable,
so no new constants can be defined on the object in userland.

PR-URL: nodejs#19813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@targos targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v10.x labels Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.