refactor(iroh-blobs)!: make TempTag non-Clone #2338
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I found that in most places it is not needed, and if somebody needs them to be clone they can always wrap them in an Arc. Also, this complicates extending the concept of temp tags via the rpc boundary. With this change if will be possible to use the same TempTag type both in the low level blobs store API and in the higher level blobs API of iroh.
rklaehn
changed the title
| @@ -66,7 +66,9 @@ impl TempCounterMap { | |||
|
|
|||
| fn dec(&mut self, value: &HashAndFormat) { | |||
| let HashAndFormat { hash, format } = value; | |||
| let counters = self.0.get_mut(hash).unwrap(); | |||
| let Some(counters) = self.0.get_mut(hash) else { | |||
| return; | |||
There was a problem hiding this comment.
when would this happen?
There was a problem hiding this comment.
If you decrement something that has not been incremented before. It should never happen, but if it happens it's better to just ignore it than to panic. Given that messages that trigger a dec go via the rpc boundary I don't think it is safe to write unreachable here.
Maybe a warning?
There was a problem hiding this comment.
given that we don't expect this happening, putting in a warning would be good, so we know if it starts happening
dignifiedquire
approved these changes
Jun 5, 2024
dignifiedquire
changed the title
dignifiedquire
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
refactor(iroh-blobs): Make TempTag non-Clone
I found that in most places it is not needed, and if somebody needs them to be clone they can always wrap them in an Arc.
Also, this complicates extending the concept of temp tags via the rpc boundary.
With this change if will be possible to use the same TempTag type both in the low level blobs store API and in the higher level blobs API of iroh.
Breaking Changes
Changes the signature of TempTag::new to take a
Weak<dyn TagDrop>insteadof an Arc. This will affect projects that write their own store
impl, so none that I am aware of. Nevertheless it is a breaking change.
The high level rpc API is not affected, since it does not know temp tags (yet).
Notes & open questions
Note: this is just part 1/x of changes needed to extend temp tags to the rpc api. On its own it does not provide much value.
Note2: you could be even more radical and give a TempTag a lifetime and a reference to its store. But I am pretty sure that this would not play well with creating bindings and might even be inconvenient from pure rust.
Change checklist