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

Add method to terminate a worker #420

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JonasAlaif
Copy link

Adds a native_worker: RefCell<Option<DedicatedWorker>> field to WorkerBridgeInner to allow immediately terminating a worker. Since there is no way to tell if a worker has been terminated from a web_sys::Worker, uses the wrapping Option to indicate this.

Also removes some redundant Rc. Now there are only two Rc-wrapped objects:

  • the WorkerBridgeInner instance shared between all forked bridges and (weakly) with the message receive handler.
  • the callbacks shared between a WorkerBridge and (weakly) the WorkerBridgeInner.

Fixes #408.

@JonasAlaif
Copy link
Author

I imagine the CI will be able to run after merging #419

@JonasAlaif
Copy link
Author

JonasAlaif commented Dec 7, 2023

Not sure if I should bump the version within the PR or if that's done later?

@ranile
Copy link
Collaborator

ranile commented Dec 8, 2023

Not sure if I should bump the version within the PR or if that's done later?

I've merged those changes. You can get the latest changes from master

Copy link
Collaborator

@ranile ranile left a comment

Choose a reason for hiding this comment

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

CHANGELOG.md in the root of the repo also needs to be updated. You can add an entry for latest version

crates/worker/Cargo.toml Outdated Show resolved Hide resolved
@JonasAlaif
Copy link
Author

Awesome, thanks! I've updated the changelog, but I'm not sure if you wanted me to add a new latest heading instead.

Copy link
Collaborator

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

I am not sure if we should support termination.

Threads in Rust cannot be terminated at all without explicitly having a synchronous mechanism sending signals to them and they exit themselves.
A Worker is technically a thread and I feel it also should not support termination.

Terminating the worker would render the remaining bridges “poisoned”, however the current bridges simply drops any new messages and does not report the termination.

In addition, if ReactorBridge::next().await or OneshotBridge::run().await is called, they will be blocked forever.

@JonasAlaif
Copy link
Author

JonasAlaif commented Dec 8, 2023

Thanks for taking a look. You do have a good point regarding the no-terminaton-support of Rust threads, which I hadn't realized. However, after looking at some discussions relating to this (e.g. here) I don't think the same applies here. For threads, the concern is that if I e.g. lock a mutex then I can break invariants as long as I restore them before releasing the lock, but if I can be terminated then the lock will be released while some invariants are still broken. The Workers here don't have the same concurrency/shared memory issues: in this regard, they are much more like separate processes. So then the argument becomes more "that Workers should have a similar API" rather than "Rust threads don't have termination due to a key underlying reason, and so we avoid the feature for the same underlying reason".

Regarding "poisoned" bridges: I think that a good solution would be to have send return some indication if it dropped the message (e.g. a Result). I just didn't want to change the API in this PR so instead added the is_terminated method which one can use to check if a bridge is poisoned. I'm happy to implement the change to the send method as well, though.

Regarding OneshotBridge I agree, its API doesn't work with terminate and I think it probably shouldn't be added there. For ReactorBridge I'll have to think about it a bit more (maybe somehow using self.rx.close()?)

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.

Allow calling terminate on workers
3 participants