Idea for Rust ownership control

[bernhard-musch]

What follows is an idea / a proposal. It may turn out not to be applicable or smart because I missed something. Let me know what you think.

current state

Data structures such as Group can be cloned and copied around freely, and they do not carry lifetimes. This makes it easy to work with them. In the library code, I can see that a Group is merely a handle to an object of the underlying HDF5 library. This is a light weight library architecture that one would probably love to keep.

However, as a logical consequence, even member functions that modify the data structures, such as Group::create_group take &self as argument. In the implementation of functions like Group::create_group , we find remarks // TODO: &mut self?. I think this might be indicative of a deeper issue. Normally, the appearance of &mut self automatically enables the compiler to enforce thread-safety. But in this case, taking&mut self as first argument would not help, because Group is Clone and so multiple instances could still mutate the same data.

To prevent data races in multi-threaded applications, the library uses reentrant mutexes. So at least every hdf5 operation on the file becomes atomic. However, the library user is left alone with the task to maintain consistency. Multiple threads can work on the same Group in parallel, which could result, for example, in an inconsistent set of attributes. That's not a hard crash or security hole, but still hard to debug.

It seems that at the moment, the library does not take full advantage of the rust ownership model, yet.

the basic idea

Keep Group (and similar data structures) cloneable. Keep the &self arguments, but for every read operation, add another argument file: &File, and for every write operation, add the argument file: &mut File. Make sure File is not Clone.

For example:

impl Group {
   pub fn group(&self, file: &File, name: &str) -> Result<Self>
   pub fn create_group(&self, file: &mut File, name: &str) -> Result<Self>
}

why this might work

From the philosophical point of view, let's think of Group (and similar data structures) as an index into an HDF5 file. Yes, an index should be cloneable. But to do something using the index, we need access to the file, either for reading or (exclusively) for writing. The new API would make this explicit.

Since File cannot be cloned, the compiler can keep track of it. Rust will automatically ensure that modifying the file can only happen with that one exclusive instance of the file, and that no thread can be reading the file at the same time. So maybe you could even get rid of the reentrant mutexes (but just maybe, because I do not know enough about the HDF5 library).

It should now be easier to keep the data consistent for library users. Library users are now forced to keep the File around, and modifying it can only happen in places with exclusive access to it.

Of course, should library users really need the flexibility, library users could put the file in an Arc<Mutex<File>>. Then multiple threads can access the file. But even then, the mechanism of sharing this access becomes more explicit: The library user has to lock that mutex to gain acces to the file. Automatically, the library user will tend to do all the operations that belong together before unlocking the mutex again.

the ugly detail

To make it work safely, we have to ensure that the File reference that is passed into member functions belongs to the right file. This can be achieved by giving each file an ID. The ID could be the existing HDF5 ID, or an extra ID created by the hdf5-metno crate. Every object opened in this file should also keep a copy of that File-ID.

For example, the new definition of Group could now be

#[derive(Clone)]
pub struct HandleWithFileID {
   handle: Handle,
   file_id: Handle,
};

#[repr(transparent)]
#[derive(Clone)]
pub struct Group(HandleWithFileID);

At the beginning of Group::group or Group::group_create we need to check that file.0 and self.0.file_id are the same ID. Otherwise we return an error or panic.

Why it's probably OK to panic? Again, think of Group as being an index into the file. Using this index with the wrong file is the same thing as an "index out of bounds" exception. By default, that is a panic in Rust. It's always a bug in the program, never an exceptional state of the application.

The file-ID check costs some performance, but very little, and definitely less than locking a mutex.

The file-ID check is symptom of a compromise we are making. We choose to work with something like indices, rather than true rust references (which would carry lifetimes and could be fully tracked by the compiler). And if these "indices" are used in the wrong context, that will only show up at runtime.

conclusion / disclaimer

I don't know if my idea really makes sense. If it does, I would be happy if I could be of help with this note.

Best regards and thanks to the library authors for their great work!

[bernhard-musch]

Closing this because 'Issue' might be the wrong place for this. Sorry.