vorner / arc-swap

Support atomic operations on Arc itself
Apache License 2.0
746 stars 31 forks source link

feature request: try_rcu / some return value to the caller of rcu #94

Open rbtcollins opened 7 months ago

rbtcollins commented 7 months ago

Sometimes the closure passed to rcu() needs to be observable to the caller.

e.g.

i = **as.load()
as.rcu(|i| if *i > 10 {  i }  else {*i + 1});
if i == **as.load() {
   Err("hit the limit")
} else {Ok(())}

But this is clearly racy as we are re-reading.

I would love to be able to do:

as.try_rcu(|i| if *i > 10 {     Err("hit the limit")  }  else { Ok(*i + 1)});

or some similar construct, where the computation within the closure is able to signal to the caller.

Would you like a patch?

vorner commented 7 months ago

Hello

To be honest, I kind of don't understand what you're trying to say/accomplish. Maybe you could sketch the signature of that try_rcu?

Does it help that the rcu returns the previous value or that the closure passed in is FnMut, therefore you can „export“ any notes outside, like

bool changed = false;
as.rcu(|i| if (*i > 10) { i } else { changed = true; *i + 1 });
if changed { ... }

Anyway, sometimes I feel like the rcu itself is already a bit too complicated bit of API, considering the lowlevelness of the whole thing… therefore I don't really enjoy the idea of adding something even more complex. Isn't it enough that anything like that can be quite easily built with compare_and_swap (with inspiration by looking at the source code of rcu)?

ilammy commented 6 months ago

I've been using the following extension trait for a while to implement try_rcu.

```rust pub trait ArcSwapExt { /// [`ArcSwap::rcu`](arc_swap::ArcSwap::rcu), but with Result that short-circuits on error. fn try_rcu(&self, f: F) -> Result where F: FnMut(&T) -> Result, R: Into; } impl ArcSwapExt for arc_swap::ArcSwapAny where T: arc_swap::RefCnt, S: arc_swap::strategy::CaS, { fn try_rcu(&self, mut f: F) -> Result where F: FnMut(&T) -> Result, R: Into, { fn ptr_eq(a: A, b: B) -> bool where A: arc_swap::AsRaw, B: arc_swap::AsRaw, { let a = a.as_raw(); let b = b.as_raw(); std::ptr::eq(a, b) } let mut cur = self.load(); loop { let new = f(&cur)?.into(); let prev = self.compare_and_swap(&*cur, new); let swapped = ptr_eq(&*cur, &*prev); if swapped { return Ok(arc_swap::Guard::into_inner(prev)); } else { cur = prev; } } } } ```

ArcSwap's API has been stable for years, so was this code. Would be nice to move it here, but it's not a big deal to keep it around like that.

My use case is fallible updates. For example, adding an item to a persistent collection wrapped in ArcSwap or failing and not changing anything if that's a duplicate.

vorner commented 5 months ago

Hmm, now I see the purpose.

My little worry here is, if it works for Result<.., ..>, should it also work for Option and for futures, etc. Unfortunately, Try is not stable yet :-(.

So I have to wonder if:

None of them looks like the ideal solution, but I'm open to your views on what to do here.

rbtcollins commented 5 months ago

I don't have a strong opinion on the details, other than it would just be nice to have the feature.

Result is more general than Option - Result<T, ()> can model an Option, but not vice versa, so my suggestion would be to either implement just Result until Try is stable, and then revisit after stabilisation.

vorner commented 4 months ago

OK, that's fair. That being said, I'm not 100% sure extanding that try_rcu for the future Try would be a semver-compatible change at that point. But we can leave it until then.

Would you send a PR?

(And sorry that my answer took so long, too many other things to attend to…)