refactor: Remove slot mutex and simplfy per blob state #104
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh-blobs/pr/104/docs/iroh_blobs/ Last updated: 2025-08-05T11:27:10Z |
| /// | ||
| /// You are not allowed to clone the state out of a task, even though that | ||
| /// is possible. | ||
| fn ref_count(&self) -> usize; |
There was a problem hiding this comment.
This isn't really needed - just a safeguard to alert people if they clone the state out of a task.
Not sure if I should keep it.
There was a problem hiding this comment.
This trait isn't exposed publicly.
So "people" would be us I guess.
What could happen if we accidentally kept a clone of the state? I guess it potentially gets reset via internal mutation.
I'm unsure, too.
I guess everyone who's working with this will know that entity states get recycled - that's the whole point.
So ... maybe not really needed?
rklaehn
changed the title
The longer term plan for this is to give people something to implement if they want their own store, so they don't have to do everything by themselves!
| } | ||
| } | ||
| .instrument(span) | ||
| }) | ||
| .await; | ||
| if let Some(task) = task { | ||
| tasks.spawn(task); |
There was a problem hiding this comment.
IMHO it would make more sense to .instrument here instead of above. I don't think there currently are, but in theory there could be logs between the callback to spawn and the future returned from spawn that wouldn't be instrumented properly otherwise.
| .thread_name_fn(|| { | ||
| format!( | ||
| "iroh-blob-store-{}", | ||
| THREAD_NR.fetch_add(1, Ordering::SeqCst) |
There was a problem hiding this comment.
| THREAD_NR.fetch_add(1, Ordering::SeqCst) | |
| THREAD_NR.fetch_add(1, Ordering::Relaxed) |
No need for SeqCst here.
| /// | ||
| /// You are not allowed to clone the state out of a task, even though that | ||
| /// is possible. | ||
| fn ref_count(&self) -> usize; |
There was a problem hiding this comment.
This trait isn't exposed publicly.
So "people" would be us I guess.
What could happen if we accidentally kept a clone of the state? I guess it potentially gets reset via internal mutation.
I'm unsure, too.
I guess everyone who's working with this will know that entity states get recycled - that's the whole point.
So ... maybe not really needed?
| "Log must contain alternating wakeup and shutdown events" | ||
| ); | ||
| if log.len() % 2 != 0 { | ||
| println!("{log:#?}"); |
Description
Before, the state we kept per hash was a bit convoluted. The entry point was a Slot, which contained a tokio mutex to cover the async loading of an entry state from the metadata db. Then inside that, there was the actual entry state, wrapped in an option to cover the case that we don't have anything about the hash.
This was working, but it was an arc in an arc in an arc, and also led to bugs in the case of trying to export a blob that doesn't exist.
Now all these states are flattened into a single enum, and we can easily define what should happen when we e.g. do an export of an entry that does not exist - return an appropriate io error.
Breaking Changes
None
Notes & open questions
Note: you could remove the Initial and Loading state by using a tokio::sync::OnceCell. But that is a true sync primitive, and we want to use an AtomicRefCell, and also using a OnceLock would come with its own sync primitive (a semaphore) just for init, and then we have our own one for the non-load state changes. I don't think it is that bad...
Note: a lot of changes are to add the wait_idle fn in the rpc protocol, which isn't really related to the changes in the PR but just a way to get rid of some flaky tests.
Change checklist