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

feat(node/cluster): cluster module for Node compat #2271

Merged
merged 30 commits into from
Jan 12, 2023

Conversation

cmorten
Copy link
Contributor

@cmorten cmorten commented May 26, 2022

Implements majority cluster module for Node compat.

Tests and full functionality not implemented owing to missing:

Which are likely blocked by denoland/deno#13566 so can be implemented in a follow-up.

node/cluster.ts Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member

@cmorten are there any blockers for this PR?

@cmorten
Copy link
Contributor Author

cmorten commented May 31, 2022

@cmorten are there any blockers for this PR?

The cluster module is basically there but it relies on process.send() (REF: https://nodejs.org/api/process.html#processsendmessage-sendhandle-options-callback) and a few other bits on process that haven’t been implemented yet, so just need to find some time to explore and port that across. Involves some IPC channel stuff and not sure yet if that’ll block it?

Guess similar to denoland/deno#12879

@cmorten
Copy link
Contributor Author

cmorten commented May 31, 2022

Hmm maybe https://github.com/denoland/deno/pull/13566… worst case can tidy up with some notImplemented() placeholders and is at least a step closer

@@ -57,7 +57,7 @@ for await (const path of testPaths) {
"--unstable",
"--no-check",
"--v8-flags=" + v8Flags.join(),
requireTs,
"--compat",
Copy link
Contributor Author

@cmorten cmorten May 31, 2022

Choose a reason for hiding this comment

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

Required to avoid errors with forked processes which start complaining because of the .ts extension - would need the require helper file to be .mjs for it to work... or just use --compat here (which opted for).

Copy link
Member

Choose a reason for hiding this comment

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

I prefer .mjs here. It's more important to check that non-compat deno can consume std/node correctly in my view

@cmorten cmorten marked this pull request as ready for review May 31, 2022 19:15
@kt3k
Copy link
Member

kt3k commented Jun 10, 2022

@cmorten Can you somehow get rid of circular dependency of net.ts -> cluster.ts -> internal/cluster/child.ts -> net.ts?

(circular dependency can be checked with the command ./_tools/check_circular_deps.ts

@cmorten
Copy link
Contributor Author

cmorten commented Jun 10, 2022

@cmorten Can you somehow get rid of circular dependency of net.ts -> cluster.ts -> internal/cluster/child.ts -> net.ts?

(circular dependency can be checked with the command ./_tools/check_circular_deps.ts

Yeah aware 😅 Had tried but it’s not a simple one to fix… suspect might have to deviate somewhat from the Node file structure, but haven’t had time recently to try and attack!

Will put this back in draft in the meantime as it’s not ready without this solved.

@cmorten cmorten marked this pull request as draft June 10, 2022 06:42
@iuioiua
Copy link
Contributor

iuioiua commented Dec 16, 2022

Hi @cmorten, is there anything we can do to progress this PR? If you cannot do so, I'm sure someone will be happy to take it on.

@cmorten
Copy link
Contributor Author

cmorten commented Dec 20, 2022

Hi @cmorten, is there anything we can do to progress this PR? If you cannot do so, I'm sure someone will be happy to take it on.

Hey @iuioiua 👋 Sorry for slow reply! Unfortunately been a matter of time (and then forgetting) that haven’t got around to finishing this off 😅. Unfortunately I don’t expect this will change in the coming weeks.

If anyone is wanting to progress or throw away these changes then please do (I seem to be making a habit with this, same happened when I implement net 🤣). I’ll see if I can get the merge conflicts sorted on my commute to make it more appetising.

IIRC the only blocker when I left it was circular deps as a result of being largely uninventive and mirroring the node implementation.

@iuioiua
Copy link
Contributor

iuioiua commented Jan 3, 2023

@cmorten, are you able to enable some native cluster-related Node tests? I'm curious whether they're affected while circular dependencies are present.

The contribution guide is in node/README.md.

@cmorten cmorten marked this pull request as ready for review January 4, 2023 12:31
`Deno.chmod()` is async
@iuioiua
Copy link
Contributor

iuioiua commented Jan 9, 2023

Hi @cmorten, is this PR ready for review? CI is green 👍🏾

@cmorten
Copy link
Contributor Author

cmorten commented Jan 9, 2023

Hi @cmorten, is this PR ready for review? CI is green 👍🏾

Yep all good to go

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

Great progress in node compatibility! Thank you for your contribution! @cmorten

@kt3k kt3k merged commit 46e2251 into denoland:main Jan 12, 2023
@lino-levan
Copy link
Contributor

Wow, this was a lot of work. Great to see that it finally landed. Incredible job @cmorten!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants