vote: future of primordials

[gireeshpunathil]

prior art for getting upto speed:

nodejs/node#30697
https://youtu.be/CYv575Bps9M
https://www.youtube.com/watch?v=XPRWSXKLdrY

• Possible goals
  • tamper-free the entire process
  • tamper-free just module loading
  • selectively, based on performance
  • something else, other than tamper-free
• Possible approaches
  • migrate to primordials
  • migrate to C++
  • something else

to reduce complexity, I have taken the liberty to remove some of the unpopular options, based on my understanding after listening to various discussions; but if you think an important option is missing, pls let me know!

don't start voting upfront, we need to better qualify the option 1 - with the relevant subset of modules . please suggest what you think those are. (pining @ljharb and @bmeck too for their input here)

voting starts in 2 weeks from now.

[targos]

What does "implement" mean?

[gireeshpunathil]

What does "implement" mean?

migrate, may be a better word?

[ljharb]

Option 1 is more like “migrate for a subset only, other core modules X this pattern” where X is “may” or “may not” or “must” or “must not”.

Option 2 is maybe “migrate for all, evaluate performance cliffs as one-offs”?

Option 3’s status quo is a bit tricky to define; it’s kind of already option 2 but there’s no consensus on it, it seems?

[gireeshpunathil]

thanks @ljharb and @targos . reworded to reflect the meaning. option 3 is different from 2 in that:

• in 2, we continue migrating, but with evaluating tradeoffs
• in 3, we just freeze everything, until a new approach arrives (such as a language semantics?)

[ljharb]

Is "freeze everything" the status quo? I thought that was "use primordials whenever possible, be careful of hot paths, and avoid primordials in hot paths".

[gireeshpunathil]

AFAIK, we stopped migration work for a while, since we started to see the performance ditch. To avoid confusion, let me reword the option to leave the migration where it is right now.

[mcollina]

would recommend using a vote similar to what we used for Promises where there were multiple choices.

[gireeshpunathil]

would recommend using a vote similar to what we used for Promises where there were multiple choices.

@mcollina - agreed in theory; just wondering how do we do it on a github issue (IIRC, promise voting was done through a google survey form or something?)

[aduh95]

For the Corepack vote (and the Promise one apparently, but I wasn't around), we used San Francisco RCV via OpaVote: #1012 (comment). Maybe we could use it again?

[gireeshpunathil]

@ljharb and @bmeck - regarding #1104 (comment) , do you have proposals for specific modules, under option 1 ? that is, the modules that you recommend as

• should be migrated
• should not be migrated
• may be / may not be migrated
  ?

[ljharb]

@gireeshpunathil personally, i want them all migrated. the subset that was referred to there afaik was "things in the loader subsystem" i believe? @bmeck can speak to this.

[BridgeAR]

I believe the TSC has to decide upon what modules should be migrated and we should have different voting options for those.

The current mentioned voting options are open for further discussions and that's something I would rather prevent.
I would like to also list pros and cons next to entries, if possible. I imply that Evaluate when a better alternative emerges is applicable to all of these options.

As such, I suggest to be very specific with our voting options. Here are a couple of suggestions:

1. Fully migrate module loaders and policy code to primordials and undo everything else (code used from other modules would not be tamper proof)
2. Fully migrate module loaders and policy code to the C++ layer and undo all primordials
   3. Migrate all modules to primordials
   4. Migrate all Node.js code to C++
3. Undo all primordials
   6. Ask the community in a similar fashion as we did with unhandled rejections
   7. Move the REPL code execution into a child process to keep the REPL from working even if the runtime itself would crash due to manipulation from the executed REPL code
   8. 1. or 2. and move the REPL code execution into a child process to keep the REPL from working even if the runtime itself would crash due to manipulation from the executed REPL code
   9. Do not port any further code to primordials but do not explicitly remove existing primordials either
4. Make sure the module loaders and policy code is tamper proof and undo all primordials in other code parts
5. Ask the community about tamper proofness, performance and Node.js contributor experience (maintainability of Node.js core).

Benefits may be one of:

a) Tamper free code loading and or execution (tampering with JS code is not going to influence the runtime itself, just the application code)
b) Performant code (primordials potentially slow down runtime code)
c) Regular JavaScript code that may be understood by most JS developers (have a lower entry barrier for new contributions)
d) TBD ...

[gireeshpunathil]

@BridgeAR - I believe these many options will confuse voters and make it harder for the TSC to converge on a stable opinion. Presence of these many (and technically may be more permutations of these?) options made us to sit and brainstorm in the first place, so as to:

• weed out some of the obviously poor choices (such as migrate all, migrate none etc.)
• identify the most prominent ones and their rationales

so as to help TSC take the most reasonable action on this.

(I won't make it to tomorrow's TSC due to its timing, but appreciate if this comment is reviewed)

[gireeshpunathil]

modified the options based on the discussion in the last TSC meeting - by separating the goal, and the approaches; thereby reducing the complexity of options.

@BridgeAR - does it look good to you? if not, pls bring your views in the upcoming sitting, then we can refine it as applicable, and then go for voting.

[BridgeAR]

@gireeshpunathil thanks for updating this. I think it's good to check for the goals. If we do that, would require a multi step voting process though, as we would have to define if a approach would in fact be able to address the goal (not each approach might in fact reach the goal).

This is in fact my main issue with primordials in the general: AFAIC they are rarely helping with the actual intend to provide a robust environment. Most applications run lots of code that is far from tamper proof and they would immediately crash as soon as built-in functions are tampered with. As such, they do not address actual user need.

Therefore I don't think the entries above are yet sufficient.

I believe these many options will confuse voters and make it harder for the TSC to converge on a stable opinion.

I personally would hope we manage with and would expect finding a limited set is in fact more difficult than having two or three voting rounds where we remove remove the least voted for options.

[ljharb]

@BridgeAR the user is in control of their application; they aren't in control of node. There is a massive difference between an unfixed fixable problem, and an unfixable problem.

[gireeshpunathil]

1. Module loading and policy would be tamper-proof. We would have to define module loader dependencies. Some dependencies are pulled in for things that might not be necessary for module loaders to work.
2. Fully migrate module loaders and policy code to C++ and undo all primordials. No ambiguity about what code has to be migrated and what could does not.
3. Migrate all modules to primordials. (Probably not feasible, in Ruben’s opinion.)
4. Migrate all code to C++.
5. Undo all primordials.
6. Ask community for feedback on performance vs. robustness vs. core maintainability/developer experience.
7. Move REPL code execution to child process to keep REPL working even if runtime would crash from tampering.
8. Do 1 or 2 in addition to 7.
9. Do not port any further code to primordials but do not remove existing primordials either.
10. (from Antoine) Use stage 1 TC39 proposal-bind-this for Migration of core modules to primordials node#30697

so these are the potential options that were presented in the TSC meeting on 8th december. we also observed that a subset of these can be weeded out (through consensus) to make the final list for voting.

Please share opinions on obvious exclusion candidates.

[aduh95]

1. We would have to define module loader dependencies.

As I said in the call, I don't think a list would work here, dependencies may change in the future, and the list would soon be outdated. My counter-proposal would be to agree on a list of tests, that could look like:

const common = require('../common');
require('./mutate_everything');

import('data:text/javascript;export{}').then(common.mustCall());

where ./mutate_everything would be a JS module (whose content can remain undefined for the moment), so we can focus on the test cases for which we want to guarantee temper-proofness. Alternatively, we could agree on a scope for those test-cases, so we can proceed to the vote even if we haven't agreed on the complete list yet.

Please share opinions on obvious exclusion candidates.

As said in the call, some options look very similar to one another (1 and 2, 3 and 4), maybe we could reformulate the problem as several questions:

1. How temper-proof do we want Node.js core to be?

• None at all.
• Module loader and policy only (no matter the perf results)
• Everything that doesn't create perf regressions (how we used to do)
• All the things (no matter the perf results)

2. How to make core temper-proof?

• primordials, the way they are used today (ClassPrototypeMethod(object, ...arguments)).
• primordials, with a build step to improve the readability (e.g. using the proposal-bind-this syntax object::ClassPrototypeMethod(...arguments)).
• migrate modules that need to be temper-proof away from JS.

And yet, I'm not very satisfied with question 1 here. There might be more areas of core that TSC members may want to see temper-proofed where performance doesn't matter (for me, that would be e.g. REPL), other ones that could be temper-proofed as long as it doesn't impact performance ( for me that would be debug tools such as console, inspector, etc.), and finally those where temper-proofness doesn't bring any value (for me that would be e.g. http). Maybe we could find a method for voting on multiple choices so we can decide once and for all where the project stands for those grey area.

[ljharb]

It also might be worth doing a ranked choice thing, since being tamper-proof isn’t binary.

[Trott]

so these are the potential options that were presented in the TSC meeting on 8th december. we also observed that a subset of these can be weeded out (through consensus) to make the final list for voting.

Please share opinions on obvious exclusion candidates.

There was consensus (at the meeting at least) that options 3, 4, and 5 are not viable decisions at this time, and that 7 was a separate issue (so therefore both 7 and 8 can be removed).

That leaves us with this list of 5 choices:

A. Module loading and policy would be tamper-proof. We would have to define module loader dependencies. Some dependencies are pulled in for things that might not be necessary for module loaders to work.
B. Fully migrate module loaders and policy code to C++ and undo all primordials. No ambiguity about what code has to be migrated and what could does not.
C. Ask community for feedback on performance vs. robustness vs. core maintainability/developer experience (before making a decision).
D. Do not port any further code to primordials but do not remove existing primordials either.
E. (from Antoine) Use stage 1 TC39 proposal-bind-this for nodejs/node#30697

[Trott]

A. Module loading and policy would be tamper-proof. We would have to define module loader dependencies. Some dependencies are pulled in for things that might not be necessary for module loaders to work.
B. Fully migrate module loaders and policy code to C++ and undo all primordials. No ambiguity about what code has to be migrated and what could does not.
C. Ask community for feedback on performance vs. robustness vs. core maintainability/developer experience (before making a decision).
D. Do not port any further code to primordials but do not remove existing primordials either.
E. (from Antoine) Use stage 1 TC39 proposal-bind-this for nodejs/node#30697

I would argue for removing Choice C. We've already done a fair amount of this. We also have the technical priorities doc that we can refer to. Choice C is very attractive because it again delays making a decision, and I think it's imperative that the TSC actually make a decision rather than drag this out for another 6 months.

I would also argue for the removal of Choice D. I think it's the worst of all worlds. Incomplete and arbitrary-seeming use of primordials is not the decision to make here.

@aduh95 Would choosing Choice E mean waiting until the proposal is implemented in V8? (Sorry if we went over this at the TSC meeting.)

If there's consensus on removing B and C, we're back down to three choices (which then might increase again as they get split into more detailed options).

[bmeck]

I'd note that an unlisted alternative is to put the tamper proof code in a loader and only guard a minimal IPC mechanism, this is what Chrome/Deno/other environments (not just JS) do. I plan on moving ESM loaders into a separate thread already due to other reasons even knowing it will be a bad performance hit for single threaded application startup times.

I'd also note that I've stated in the past I do not believe making CJS tamper proof is realistic given its design and encouragement of monkey patching in the wild. You could add a tamper safe hook in another thread that could override it entirely though (once again at a big perf penalty).

In general the idea for both of those is that we are not preventing mutation of behaviors but only allowing certain opt-in code to be able to modify it (like loader hooks, or any other hooks we add). I do think C++ also would be a bad hit if done naively, we used to have ESM written in C++ but it was ported to JS because people wanting it to be easier for new contributors to work with.

[Trott]

Here's a user having console.log() usage result in an unexpected/hard-to-understand error after some code modifies [].__proto__. Just putting this in the bucket of "real world cases where users expect the kind of robustness that primordials aims to achieve". I'm not advocating for accommodating or ignoring the use case. Just documenting it here in the relevant conversation.

Refs: nodejs/node#41151
Refs: mishoo/UglifyJS#5217

[aduh95]

@aduh95 Would choosing Choice E mean waiting until the proposal is implemented in V8? (Sorry if we went over this at the TSC meeting.)

No, it would mean authoring the code using a syntax not supported (yet? ever?) by V8 (e.g. using the proposal-bind-this syntax: myArray::ArrayPrototypePush(someValue) instead of ArrayPrototypePush(myArray, someValue)) and add a build step to transpile back to a syntax supported by V8.

[aduh95]

FWIW, Deno has somewhat recently decided to go the primordials way; I'm mentioning it here in case folks would be interested in reading the motivations that led to that choice for Deno.

Refs: denoland/deno#11224
Refs: denoland/deno#10939 (comment)

[ljharb]

@aduh95 that proposal is on the agenda for next week's meeting, fwiw, so hopefully it will move along towards being supported in v8 natively.

update: it was deferred to January's meeting, unfortunately.

[mhdawson]

I'm + 1 for the suggestion of removing option C.

I think in this case the question is much less clear than when we asked about behavior of unhandled rejections and that we won't get concrete enough data to help make the question. The result just being a delay.

[MylesBorins]

I've given a thumbs up but do plan to abstain for voting as I do not have enough context to make an informed decision about this. Please feel free to not count my thumbs up if it doesn't make sense

[jasnell]

Definite thumbs up for a vote.

[jasnell]

Ping @nodejs/tsc members... please weigh in on moving to a vote: #1104 (comment)

[targos]

Should we define what code in the error path means before the vote?

[aduh95]

Should we define what code in the error path means before the vote?

Yes it's important to clarify that. To me, that means any code that runs before (or inside) an inevitable `throw` statement. This leaves as grey area functions that are shared between non-error paths and error path, e.g.:

    throw new ERR_MODULE_NOT_FOUND(
      path || resolved.pathname, base && fileURLToPath(base), 'module');

In this case, fileURLToPath is on the error path, but it's also a public API, and I don't think it should be covered by this first vote.
Do not hesitate to ask for clarifications, or suggest a better framing, the last thing we need would be to have the vote results inapplicable because they leave too much to interpretation.

[Trott]

Now that we have enough people seconding the call for a vote, who will set up the voting mechanism? Gireesh? Antoine? Someone else?

[gireeshpunathil]

Now that we have enough people seconding the call for a vote, who will set up the voting mechanism? Gireesh? Antoine? Someone else?

I little held up on personal front right now. I am happy and thankful if @aduh95 drive this voting.

[mcollina]

Can you please clarify what the "error path" mean in the following sentence:

Do we want to make code in the error path tamper proof (and how tamper proof do we want it to be)?

[aduh95]

Can you please clarify what the "error path" mean in the following sentence:

Do we want to make code in the error path tamper proof (and how tamper proof do we want it to be)?

#1104 (comment)

Now that we have enough people seconding the call for a vote, who will set up the voting mechanism? Gireesh? Antoine? Someone else?

I'm happy to do it as long as everyone is confortable having their vote public – if the voting is private like it was for the Corepack vote, it wouldn't be fair to trust me with it given my involvement in the matter.

[mcollina]

What would it take to make the error path fully tamper proof? What changes would be needed by streams?
Why only the error path?

[targos]

Rethinking about it now... I suppose that @aduh95 suggests to do it in the error path because there are less performance concerns in that case? But aren't the upsides of tamperproofing the error path much lower than the normal path? There's not much anyone can control about the behavior of Node in there.

[aduh95]

Why only the error path?

1. We have to start somewhere, other areas of the code base can be decided in later votes.
2. To me, code on the error path is a typical example where most users would prefer reliability over performance.

What would it take to make the error path fully tamper proof?

If the vote result is that error path should be tamper proof, the difference it really makes is that PR increasing tamper proofness on the error path can be landed, PR decreasing tamper proofness on the error path can be blocked – and similarly, if the result of the vote is the error path doesn't have to be tamper proof, a PR removing primordials couldn't be blocked unless there are other concerns, and a PR using primordials to the error path would be blocked.

To be clear, whatever the result of this vote is, a PR could still be blocked if you have concerns over it (e.g. if it introduces perf issues on other areas), no matter if it increases or decreases the tamper proofness.

What changes would be needed by streams?

None I'm hoping, or we using streams extensively on the error path? (even if it is, stream being a public API, even if it's on the error path it would be out of scope for this vote, this vote result would apply for error-path-only code; a future vote can be made later for tamper proofing streams or not)

[ljharb]

fwiw, I primarily care about reliability on the non-error path; on the error path, my process is generally terminating, so I don't care as much about either reliability or performance.

[mhdawson]

+1 for vote but would be good to clarify what Make it applies to. All of Node.js, the loading path, or something else. Or possibly there will be a few votes where it will be something different each time?

[Trott]

+1 for vote

That makes 11 of us which is > 50% of the TSC. Let's do this thing!

[ronag]

Sorry... but how do I vote?

[aduh95]

Sorry... but how do I vote?

Vote is happening in #1158. Check the OP there for instructions, don't hesitate to ask questions if you have any.

[mhdawson]

Discussed in the TSC meeting this week. We agreed this issue should be closed and new issues opened for any follow on votes. Suggested candidates for opening the new issue were @aduh95. @gireeshpunathil. @BridgeAR.