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

Actor Registry #1969

Merged
merged 5 commits into from
Oct 6, 2022
Merged

Actor Registry #1969

merged 5 commits into from
Oct 6, 2022

Conversation

fulmicoton
Copy link
Contributor

No description provided.

@fulmicoton fulmicoton changed the title Weak mailbox Actor Registry Sep 19, 2022
@fulmicoton fulmicoton marked this pull request as draft September 19, 2022 06:57
@fulmicoton fulmicoton force-pushed the weak-mailbox branch 6 times, most recently from 2dcc86b to 25948e4 Compare October 5, 2022 04:38
@fulmicoton fulmicoton marked this pull request as ready for review October 5, 2022 04:40
Copy link
Collaborator

@boraarslan boraarslan left a comment

Choose a reason for hiding this comment

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

Looks great; I have two questions though.

  1. Why are we using std::sync::RwLock instead of tokio's RwLock? Is there any particular reason?
  2. Unwrapping a poisoned lock would result in a crash. Is this what we want, or do we have a recovery logic?

quickwit/quickwit-actors/Cargo.toml Outdated Show resolved Hide resolved
quickwit/quickwit-actors/src/lib.rs Outdated Show resolved Hide resolved
quickwit/quickwit-actors/src/registry.rs Outdated Show resolved Hide resolved
quickwit/quickwit-actors/src/registry.rs Outdated Show resolved Hide resolved
@fulmicoton
Copy link
Contributor Author

Looks great; I have two questions though.

  1. Why are we using std::sync::RwLock instead of tokio's RwLock? Is there any particular reason?
  2. Unwrapping a poisoned lock would result in a crash. Is this what we want, or do we have a recovery logic?

Two great questions...

I'll start by 2.
Absolutely... In tantivy and quickwit, we try to make that kind of lock usage as trivial as possible.
This means: ideally, keeping your lock guard in its simple scope (not returning it, etc).
Ideally only execute trivial non-panicking code while the lock is held. Often this means we take the lock, clone the value and unlock. This is not great from a performance standpoint but this is much easier to read proof.
It is tempting to just use a magical implementation that cannot poison but the problem is not really solved is it?
If your tokio task panic while you where writing your object you might have left your object in an inconsistent state!
Panicking on the next lock acquisition is actually better than letting readers seeing (and possibly propagating/persisting etc) that inconsistent state.

  1. The async Mutex is required if you have the lock accross an async.
    https://tokio.rs/tokio/tutorial/shared-state#on-using-stdsyncmutex

Outside of this, if something on which there are no contention like it is the case here, using sync::Mutex is fine, and actually more efficient.
By using a std::sync::Mutex I'm expressing that this is supposed to be a trivial case, and the reader should assume:

  • a small scope
  • only simple computation done (no IO for instance)

@fulmicoton
Copy link
Contributor Author

As discussed offline with @guilload , we might want a registry for service like actors that are singleton (in the Universe).
Merging this and explorring this in a different PR.

@fulmicoton fulmicoton enabled auto-merge (squash) October 6, 2022 07:18
@fulmicoton fulmicoton merged commit acb249b into main Oct 6, 2022
@fulmicoton fulmicoton deleted the weak-mailbox branch October 6, 2022 07:19
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.

3 participants