Repository navigation
vote: future of primordials #1104
Description
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.
Activity
targos commentedon Oct 18, 2021
What does "implement" mean?
gireeshpunathil commentedon Oct 18, 2021
migrate, may be a better word?
ljharb commentedon Oct 18, 2021
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 commentedon Oct 18, 2021
thanks @ljharb and @targos . reworded to reflect the meaning. option 3 is different from 2 in that:
ljharb commentedon Oct 18, 2021
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 commentedon Oct 18, 2021
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 commentedon Oct 18, 2021
would recommend using a vote similar to what we used for Promises where there were multiple choices.
gireeshpunathil commentedon Oct 18, 2021
@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 commentedon Oct 18, 2021
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 commentedon Oct 28, 2021
@ljharb and @bmeck - regarding #1104 (comment) , do you have proposals for specific modules, under option 1 ? that is, the modules that you recommend as
?
ljharb commentedon Oct 28, 2021
@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 commentedon Nov 4, 2021
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 emergesis applicable to all of these options.As such, I suggest to be very specific with our voting options. Here are a couple of suggestions:
3. Migrate all modules to primordials4. Migrate all Node.js code to C++6. Ask the community in a similar fashion as we did with unhandled rejections7. 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 code8. 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 code9. Do not port any further code to primordials but do not explicitly remove existing primordials eitherBenefits 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 commentedon Nov 10, 2021
@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:
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 commentedon Nov 21, 2021
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 commentedon Dec 2, 2021
@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 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.
37 remaining items
aduh95 commentedon Jan 11, 2022
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.:In this case,
fileURLToPathis 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 commentedon Jan 13, 2022
Now that we have enough people seconding the call for a vote, who will set up the voting mechanism? Gireesh? Antoine? Someone else?
gireeshpunathil commentedon Jan 13, 2022
I little held up on personal front right now. I am happy and thankful if @aduh95 drive this voting.
mcollina commentedon Jan 13, 2022
Can you please clarify what the "error path" mean in the following sentence:
aduh95 commentedon Jan 13, 2022
#1104 (comment)
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 commentedon Jan 13, 2022
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 commentedon Jan 13, 2022
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 commentedon Jan 13, 2022
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.
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 commentedon Jan 13, 2022
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 commentedon Jan 14, 2022
+1 for vote but would be good to clarify what
Make itapplies to. All of Node.js, the loading path, or something else. Or possibly there will be a few votes whereitwill be something different each time?Trott commentedon Jan 15, 2022
That makes 11 of us which is > 50% of the TSC. Let's do this thing!
ronag commentedon Feb 3, 2022
Sorry... but how do I vote?
aduh95 commentedon Feb 3, 2022
Vote is happening in #1158. Check the OP there for instructions, don't hesitate to ask questions if you have any.
mhdawson commentedon Feb 23, 2022
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.