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.

Sign up for GitHub

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

Jump to bottom

lib: restate the code to use primordials #36294

Closed
wants to merge 2 commits into from
Closed

lib: restate the code to use primordials #36294

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6ba8aa
lib: restate the code to use primordials
PoojaDurgad Nov 27, 2020
9201949
fixup: revert the changes
PoojaDurgad Nov 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/internal/process/execution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
'use strict';

const {
FunctionPrototype,
} = primordials;

const path = require('path');

const {
Expand Down Expand Up @@ -121,7 +125,7 @@ function hasUncaughtExceptionCaptureCallback() {
return exceptionHandlerState.captureFn !== null;
}

function noop() {}
const noop = FunctionPrototype;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this change was made. Changing this from a function to a prototype object is changing the type entirely. Additionally, a function itself shouldn't ever need to be converted to any kind of primordial.

Copy link
Contributor

Choose a reason for hiding this comment

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

TBF typeof Function.prototype === 'function', so it's not really changing the type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Even so, I don't see the benefit of changing this unless someone can point to something specifically that this would be protecting us from?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://tc39.es/ecma262/#sec-properties-of-the-function-prototype-object

The Function prototype object:

  • is itself a built-in function object.
  • accepts any arguments and returns undefined when invoked.

It seems to be strictly equivalent to function noop(){}, the only difference I can think of is that it doesn't create a new Function object and may be marginally more memory efficient.
I highly doubt there is any scenario where it would show that Node is using one or the other, so I'm neutral to this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it’s passed to setImmediate, which is obtained from require('timers') at the time the callback created by createOnGlobalUncaughtException is invoked, so if user code wraps setImmediate from timers, it can determine whether the passed function is %Function.prototype%.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mscdex ,@aduh95 - I made the changes based on this logic typeof Function.prototype === 'function' . Hence I modified the code. I am fine to revert this change, if others think it is not worth


// XXX(joyeecheung): for some reason this cannot be defined at the top-level
// and exported to be written to process._fatalException, it has to be
Expand Down