Unnoticed clone into closure

help
1

Hi there,

I'm passing a HashMap into an event handler like this:

let mut state = HashMap::new();

client.add_event_handler(|...| async move {
  // read and update `state` variable here
});

add_event_handler is typed as:

pub fn add_event_handler<Ev, Ctx, H>(&self, handler: H) -> EventHandlerHandle
where
    Ev: SyncEvent + DeserializeOwned + Send + 'static,
    H: EventHandler<Ev, Ctx>

I thought that every time a suitable event happened, my closure would be called and they would all use the same state variable to keep track of things.

But no. That was not what happened. The closure didn't remember events via state. After putting logs here and there, I finally noticed that EventHandler had Clone as its supertrait. This made the code clone my closure without me knowing about it and thus I made the wrong reasoning.

So, is there any way to avoid this mistake in the future? Is there some kind of lint to help? I think it's pretty obvious that cloning a moved-in empty collection doesn't sound reasonable. Or maybe let me know the closure is going to be Clone at the function's documentation?

2

That's one of the motivations to create GitHub - zjp-CN/term-rustdoc: [WIP] A TUI for Rust docs that aims to improve the UX on tree view and generic code. to enhance UX on generic types in documentations.

generic types enhancement

  • generic type parameters
    • list concrete candidate types that meet the trait bounds
      • from within the current pkg
      • from within the caches in database
    • list the functions/methods that
      • return generic types that hold the same trait bounds
      • return concrete candidate types
    • list the function/methods that
      • accept generic types that hold the same trait bounds :point_left: here
      • accept concrete candidate types
  • lifetime parameters
    • variance (lack of this info in json docs, but maybe not hard to have it)

:point_right: We need an expanded view for trait alias (trick).

1 Like
3

I'd put the blame on the library. FnOnce + Clone (rather than Fn or FnMut) is a pretty weird way to handle callbacks. They probably did it so that users like you don't have to write a lot of clone()s in the closure to construct your async block, but you got bitten by the non-explicitness of that.

As to noticing the problem: what I would hope to notice, through my experience with writing code that obeys Rust ownership rules, is that the async move { ...state... } block is guaranteed to take ownership of state when evaluated, so I should expect the closure to fail to compile as it only has one state to give away once. But it succeeds, so it must be allowed to be a FnOnce, so something funny is going on.

Additionally, if I am modifying state from async blocks being run multiple times, I know that the state will need to be interior mutable (because it is being accessed in a possibly interleaved fashion), so the fact that the compiler has not objected to overlapping mutable borrows is another warning sign.

5 Likes
4

Hmm, that explains why I didn't make this kind of mistakes elsewhere.

I don't quite understand this. Could it be a FnMut and reuse the owned state again and again?

Oh I see. Async closures are different in this way! But if the moved-in variable is already interior mutable, it could be a Fn instead of a FnOnce + Clone, isn't it? (I can't recall something both interior mutable and clone to independent objects however.)

@vague that surely would help! But I prefer web pages anyway, as it's nicer and can exploit browser features like multiple tabs, bookmarks, search and highlight, etc.

5
1 Like
6
  1. Remember that the closure function and the async-block future are separate things. The closure, when called, evaluates, and returns the value of, the expression async move { ...state... }. The effect of any async move { ... x ... } expression (or move || { x } too, though that is not relevant here), is to immediately move x into the future it returns.
  2. Therefore, in this specific case, we know, syntactically, that the closure cannot retain ownership of state (unless state's type were Copy, which it is not), because the closure always gives away state when it is called.
  3. Therefore, it cannot be a FnMut, because FnMuts cannot give away ownership of their captured variables and leave them in the moved-out-of/de-initialized state.

In order to accomplish this, you'd need to also use Arc or something in order to share the interior-mutable type between the multiple futures, and the Arc must be cloned in order to serve this function, so, again, we can conclude that there must be some implicit cloning going on if the code compiles.

2 Likes
7

I see, thank you! (This should be in the async tutorial, if not already. I learned async quite early so perhaps missed a lot mature docs...)

8

There is another tell in the source.
The handle_event() consumes self.
Document is explicitly hidden.

9

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.