Cannot borrow `network2` as mutable more than once at a time

1

When the project is small I usually manage to deal with rust, bu I started a big project and there is something I don't understand how to deal with.

My project is a rust implementation of
GitHub - abriotde/openhems-sample: Sample and very basic Home Energy Management System based on Home-Assistant.

My project is GitHub - abriotde/openhems-rust at rust-pb and I get the error:

error[E0499]: cannot borrow `network2` as mutable more than once at a time
  --> src/main.rs:54:6
   |
52 |             match Server::new(& mut network2, &configurator) {
   |                               -------------- first mutable borrow occurs here
53 |                 Ok(mut hems_server) => {
54 |                     network2.set_server(Rc::new(&hems_server));
   |                     ^^^^^^^^                    ------------ first borrow later used here
   |                     |
   |                     second mutable borrow occurs here

The problem occurs at top level because under, we did atomic thinks or we can report it at upper level.

It's a Home-Assistant add-on to Optimize electric consumption. Basically:
The "Server" own a "Network" with own devices "Switch" and "PublicPowerGrid". Each devices own there "Feeder" witch are way to get informations from Home-Assistant (or not if const). Network own too a HomeAssistantAPI witch update the Network.
Server should loop and update network often.
Beside an OffpeakStrategy should choice the best to do on network : when to switch on/off devices.

I am pretty sure I should change some lifetime and maybe re-think somethink in architecture. But what?

2

It rather looks like the Server exclusively borrows the Network, rather than owning it. Maybe there's your issue.

3

You're right, it look better. I have just done few changes, it seam work. It's too early to say if it's the solution. But it could be.

4

You're overusing references (and misusing lifetimes, but getting rid of the former will probably fix the latter). For instance, almost every data structure you have has lifetimes, some have multiple, and you also have fields like &'a OtherThing<'a, 'a>.

Rust references are for short-lived borrows, not long-lived data structures. Few -- probably none -- of your data structures should have lifetime parameters. Figure out how to rewrite them so that they take ownership, not references.

I didn't read enough to be sure, but you may have circular relationships among your datastructures. That may require further refactoring. It's not necessarily a show-stopper, but if possible, prefer a clear tree of ownership.

2 Likes
5

I had another look, and yeah, you're trying to use Rust references like they were Python object values or such, so that multiple data structures can all simultaneously hold on to a handle to the same Network, etc. In the end, everything ends up having a handle to everything else through one path or another.

That's not what Rust references are for. The closest thing would be Arc<Mutex<_>> or Rc<RefCell<_>>. Or just Rc<_> or Arc<_> if you don't need &mut _s.

So one option would be to convert most your reference-holding data structures to use those instead.[1] Look into Weak for cycles so that you don't leak memory. You might have to contend with panics (RefCell) or deadlocks (Mutex) instead of borrow-checker errors, depending on how exactly things are used.

LazyLock or similar is another alternative for global resources.

A third option is to use something besides the "everything has a handle to everything else" model, which I touch on below. It may be a larger redesign. It would probably be more idiomatic.

In more detail, you've created a big self-referencial ball of mud.

Here are a bunch of probable[2] reference cycles I found:

Network Server
Network Server OffPeakStrategy
Network HomeAssistantAPI
Network NodesHeap PublicPowerGrid NodesBase
Network NodesHeap PublicPowerGrid NodesBase SourceFeeder HomeAssistantAPI
Network NodesHeap PublicPowerGrid NodesBase Feeder SourceFeeder HomeAssistantAPI
Network NodesHeap Switch NodesBase
Network NodesHeap Switch NodesBase SourceFeeder HomeAssistantAPI
Network NodesHeap Switch NodesBase Feeder SourceFeeder HomeAssistantAPI

Instead of going all-in on reference counting, you could instead try to redesign things to have a clear ownership story. Even if you end up with Network in an Rc<Mutex<_>> or such, you could at least reduce the need for reference counting everything.

Let's look at a specific example:

pub struct HomeAssistantAPI<'a, 'b:'a> {
    network: Option<&'b Network<'a>>,
    // ...
}
A side bar about the lifetime bounds (click to expand)

The presence of &'b Network<'a> in a fields means that there is an implicit 'a: 'b bound. In combination with your explicit 'b: 'a bound, that means that 'b and 'a must always be the same lifetime. Probably you added the explicit bound because the compiler suggested it somewhere, and the compiler probably suggested it because the self-referencial nature of your data structures made it required somewhere.

This pattern where the two lifetimes have to be the same is repeated on all of your two-lifetime structs I looked at except NodesHeapIterator. Should you make them all one lifetime? Probably it would be fine to do so, but it's hard to say with certainty, so I'd just leave it be until you -- hopefully -- refactor all or most of them away.

In any case, the "lifetimes were forced to be the same" pattern is a yellow flag to be aware of.

At least in home_assistant_api.rs, it doesn't look like you use this field. So get rid of it. Then in places where you might have used it, take &Network<'_>[3] as an argument instead.[4]

Then pick another lifetime-carrying struct, especially if it's part of a cycle, and see if you can get rid of the lifetimes on it, too. You'll have to figure out what types own resources and what types take borrows of them as arguments in the process.

Some other drive-by comments follow.[5]

pub struct Network<'a> {
	server: Option<Rc<&'a Server<'a, 'a>>>

Rc<&T> offers no real benefit over &T.

pub trait EnergyStrategy<'a, 'b:'a, 'c:'b> {
	fn new(network:&'c Network<'a>, id:&str, config:&LinkedHashMap<Yaml, Yaml>) -> ResultOpenHems<OffPeakStrategy<'a, 'a>>;

The implicit 'a: 'c from the nested lifetime in new, plus the explicit 'c: 'b, 'b: 'a from the trait, means that 'a and 'c (and 'b) have to be the same in the call to new. Probably this mostly just mirrors what's going on with your structs.

However:

  • A trait with this many lifetimes is a yellow flag generally
  • 'b isn't used for anything else, so drop it
  • Probably you can put the remaining lifetime(s) you need on new instead, which I recommend

(Though this may all be short-term busy work as you refactor lifetimes away anyway.)

impl<'a, 'b:'a, 'c:'b> fmt::Display for HomeAssistantAPI<'a, 'b> {

You generally don't need to repeat lifetime bounds from the struct definition on implementations. And why is there a 'c here? Most of your implementations should not involve bounds:

impl fmt::Display for HomeAssistantAPI<'_, '_> {

// Or if you actually need to name the lifetimes
impl<'a, 'b> SomeTrait<'a, 'b> for HomeAssistantAPI<'a, 'b> {
    fn some_method(&self, whatever: &'a Whatever<'b>) {

  1. It probably makes sense for NodesHeapIterator to hold a reference, but I didn't check this in depth. It's your only lifetime-carrying type that's not mixed up with all the others I talk about below. ↩︎

  2. i.e. based on the types and their fields ↩︎

  3. or in the future &Rc<Network> or Rc<Network>, etc, if you need that to create some return type value that holds an Rc<Network> ↩︎

  4. Or use the global, if you went that route. ↩︎

  5. They won't matter much in your project after a refactor to get rid of most the named lifetimes, but might still be educational. ↩︎

1 Like
6

Thank you. I'd like too try the idiomatic way...
There is so much lifetimes because I tried many thinks...
You're right, home_assistant don't need network when I check Python version : openhems-sample/src/openhems/modules/network/driver/home_assistant_api.py at main · abriotde/openhems-sample · GitHub
But it update an internal "cache" witch is looked by "Feeders" from "Nodes" on "Network".
But sometimes it need for further purpose, but not implemented yet in rust version (implemented in Python's one).

7

It's now better. I have reduce drastically the number of references in structs. There is just HomeAssistantAPI in Network and SourceFeeder. And Network in Server and OffPeakStrategy. And I use Rc<RefCell<>> for them except NodesHeapIterator.
But I get some more problems. I think because I use network, so I borrow the Network and inside I want to borrow HomeAssistantAPI witch is in Network.

What do you mean by :
A third option is to use something besides the "everything has a handle to everything else" model, which I touch on below. It may be a larger redesign. It would probably be more idiomatic.
It would be full of references... a mess no? Do you think to a design pattern?

8

I mean try to avoid having things like circular references (be they actual references or Rc<_>), where e.g. a Network holds a handle to a Server which owns some OffPeakStrategy which all hold a handle to the Network. Instead, pass references in as parameters to methods. So perhaps OffPeakStrategy's methods take a &Network or &mut Network when needed.

Or if some resource is truly global, like your program will only ever have one and every other type wants access to that instance, something like LazyLock can be used instead.