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

Broadcast to workers #113

Closed
wants to merge 3 commits into from
Closed

Conversation

mariusandra
Copy link

Hi team Piscina,

We're using piscina to power the the PostHog plugin server and have a requirement to implement. We need to be able to broadcast a "reload" task to all workers.

This is my first attempt at implementing this and would be happy to make whatever changes you think are needed.

Would you be willing to accept this PR when it's ready and what should I do to improve it?

@jasnell
Copy link
Collaborator

jasnell commented Mar 1, 2021

Interesting idea for sure. Just want to repeat back to make sure I understand... The basic idea is to submit a task that is broadcast out to all existing workers rather than picked up by a single available worker... correct? Definitely not opposed to the idea but there are some complexities there... lemme stew on it a bit before responding in detail :-)

@mariusandra
Copy link
Author

That's indeed the case. We have a system where we must run a number of custom user-provided plugins (JS code running in a VM) on events that pass through our pipe. We launch a bunch of workers via piscina and load all plugins inside each worker, ready to process whichever event comes in.

The problem now is that if any plugin needs to be reloaded, we have no choice but to take down the entire cluster and recreate it from scratch. Instead I'd like to send each running worker a "reload" event, which will cause it to fetch a diff from the server (which new plugins were enabled, etc), and then just reload or launch only the VMs that need a change.

@jasnell
Copy link
Collaborator

jasnell commented Mar 4, 2021

Ok! Definitely an interesting case. I haven't had a chance to review the PR (and likely won't be able to until Monday) but it's on my list to look at.

@addaleax ... any thoughts here?

@addaleax
Copy link
Collaborator

addaleax commented Mar 5, 2021

I think this is fine if it’s being used with reload semantics – although I also think that, once they’re in an LTS release branch, BroadcastChannels probably fill this use case a bit better :)

@mariusandra
Copy link
Author

What's BroadcastChannel? :)
And what can we do to get this in?

@mceachen
Copy link

mceachen commented Mar 25, 2021

What's BroadcastChannel? :)

Presumably https://nodejs.org/dist/latest-v15.x/docs/api/all.html#worker_threads_class_broadcastchannel_extends_eventtarget

@jasnell
Copy link
Collaborator

jasnell commented May 15, 2021

Closing this as I believe using BroadcastChannel is a better option.

@jasnell jasnell closed this May 15, 2021
@mariusandra
Copy link
Author

Hmm, that's a bit of a shame.

Seeing "Stability: 1 - Experimental" and that it's only available from node 15.4.0 unfortunately means we can't use this. Thus we have no other option but to keep using our fork (@posthog/piscina on npm) for the near future :/.

Thank you for the awesome library though!

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.

4 participants