Circuit breaker #37
Circuit breaker #37
Conversation
plazmoid
force-pushed
the
DATA-1853_circuit_breaker
branch
from
edee4bc to
4e3f0f6
Compare
plazmoid
force-pushed
the
DATA-1853_circuit_breaker
branch
from
894f336 to
e2190db
Compare
plazmoid
force-pushed
the
DATA-1853_circuit_breaker
branch
from
e2190db to
df2138a
Compare
|
|
||
| Ok(CircuitBreaker { | ||
| state: RwLock::new(CBState { | ||
| data_source: Arc::new(init_fn()?), |
There was a problem hiding this comment.
What is the use of putting Arc under Mutex? It is thread-safe by design.
| let state_read_lock = self.state.read().await; | ||
| let result = query_fn(state_read_lock.data_source.clone()).await; | ||
|
|
||
| drop(state_read_lock); |
There was a problem hiding this comment.
This looks silly - the thread-safe dasta structure such as Arc was put under mutex, even async mutex, and now we need to lock that mutex just to clone the Arc.
There was a problem hiding this comment.
CB is designed to interfere client's code as less as possible. RwLock is used here to provide interior state mutability, without it, the end client will suffer while refactoring &self to &mut self.
Arc is used to pass repo to query function. I'm not using & due to lifetime problems (query_fn is async)
| sync::Arc, | ||
| time::{Duration, Instant}, | ||
| }; | ||
| use tokio::sync::RwLock; |
There was a problem hiding this comment.
What is the justification of using async mutex here? It seems that it is unnecessary.
Also, what is the justification to use RwLock? It seems that every attempt to lock this mutex requires write access, so a regular Mutex would do.
There was a problem hiding this comment.
We can read-only from rwlock if current and previous queries were successful, so we don't need to obtain write lock and reset state every time.
See upper comm about rwlock's purpose here
| /// let cb = CircuitBreaker::builder() | ||
| /// .with_max_timespan(Duration::from_secs(1)) | ||
| /// .with_max_err_count_per_timespan(5) | ||
| /// .with_init_fn(|| Ok(Repo)); |
There was a problem hiding this comment.
What is the use of this init_fn? Why not to just create that "datasource" once?
What is the real-life example when this overcomplication is really necessary?
There was a problem hiding this comment.
deadpool_diesel stops working after failed db query, so its needed to be reinitialized
| Fut: Future<Output = Result<T, S::Error>>, | ||
| { | ||
| let state_read_lock = self.state.read().await; | ||
| let result = query_fn(state_read_lock.data_source.clone()).await; |
There was a problem hiding this comment.
Calling (and awaiting) an external async fn while holding lock makes this query() effectively single-threaded which is unacceptable in a real-world application.
There was a problem hiding this comment.
rust docs:
An RwLock will allow any number of readers to acquire the lock as long as a writer is not holding the lock.
So, read is not blocking, and write will block thread only if query fn failed
Resolve DATA-1853