gtk-rs / gtk-rs-core

Rust bindings for GNOME libraries
https://gtk-rs.org/gtk-rs-core
MIT License
279 stars 110 forks source link

Figure out and document the thread-safety story #41

Open SimonSapin opened 5 years ago

SimonSapin commented 5 years ago

In cairo

Many objects in cairo are manipulated through pointer types like cairo_surface_t * and are reference-counted, with functions like cairo_surface_reference and cairo_surface_destroy. (Despite what the name suggests, the later does not free the object unless the refcount reaches zero.)

Unfortunately I couldn’t find any documentation about what is considered correct use of these objects or not, in a multi-threaded application.

I think we can conservatively assume that cairo should be thread-safe when any object created through the public API is only manipulated on the same thread it was created on. Some commit messages and entries in the NEWS file mention for example adding a mutex around the global font cache.

However it is not clear to me what objects can be shared across threads under what conditions, if at all. The NEWS file includes “Making reference counting thread safe” which I assume is similar to using Rust’s Arc instead of Rc, which would be relevant when objects are shared.

In cairo-rs, status quo

Cairo objects are exposed in the Rust API as wrapper structs that implement Clone and Drop by incrementing and decrementing the reference count.

As of cairo-rs 0.6.0 (since PR https://github.com/gtk-rs/cairo/pull/233) most methods take a shared &self rather than exclusive &mut self. This reflects the fact even with exclusive (&mut) access to a reference (copy of the pointer) to an object, there can be other references/pointers to the same object so access is effectively shared.

These structs wrapper are !Sync and !Send as a side-effect of containing a *mut raw pointer. I think it would be good to have explicit negative impls, to show intent to someone reading the code and to have rustc error messages mention e.g. cairo::Surface instead of *mut cairo_sys::cairo_surface_t. But negative impls are not stable yet: https://github.com/rust-lang/rust/issues/13231.

All of the above is not obvious, as shown by https://github.com/gtk-rs/cairo/issues/192. It would be good to have high-level documentation that explains it. (By the way, I’ve found https://github.com/gtk-rs/lgpl-docs but it’s not clear how it relates to this repo. Does the release script use whatever is the latest docs commit at the time? Are new docs written for Rust bindings also under LGPL?)

More flexibility in cairo-rs?

There’s desire to do more with threads: https://github.com/gtk-rs/cairo/issues/175, https://github.com/gtk-rs/cairo/issues/226. If we can get answers on what is considered correct use of cairo’s C API or not (and ideally have them documented upstream), maybe we can relax these !Send and !Sync negative impls to some extent?

Or, one might imagine a Send wrapper that checks if the reference count is 1. However some object might internally own references to other objects, and the latter could end up being incorrectly shared across threads. For example, the existence of cairo_get_target shows that a cairo_t context owns a reference to a cairo_surface_t.

sdroege commented 5 years ago

Surfaces are neither Send nor Sync because of the reference counting and interior mutability. If they were not reference-counted, some surfaces could be Send (image surface for example), but others are not (Win32 surfaces for example can only be used from the thread where they were created).

sdroege commented 5 years ago

Or, one might imagine a Send wrapper that checks if the reference count is 1. However some object might internally own references to other objects, and the latter could end up being incorrectly shared across threads. For example, the existence of cairo_get_target shows that a cairo_t context owns a reference to a cairo_surface_t.

This is indeed a problem and can easily happen if you use Cairo structs in combination with other libraries like GTK. You never know if someone else keeps a reference around to your thing.

If someone has an idea how to implement something more flexible with the given constraints

piegamesde commented 3 years ago

https://developer.gnome.org/gdk3/stable/gdk3-Threads.html

GLib is completely thread safe (all global data is automatically locked), but individual data structure instances are not automatically locked for performance reasons. So e.g. you must coordinate accesses to the same GHashTable from multiple threads.

I'm confused. Doesn't this mean that it should be possible to implement a mechanism for sharing data across threads in a safe manner?

GuillaumeGomez commented 3 years ago

You mean like channels? Which are also provided?

piegamesde commented 3 years ago

Sorry, I was not specific enough. With "data", I meant objects like GdkPixbuf or PopplerDocument. As far as I understand it, channels are no help here because they require the data to be Send.

GuillaumeGomez commented 3 years ago

To be exact, the objects aren't thread-safe, but I see no issue about sending their data though.

sdroege commented 3 years ago

GdkPixbuf values and others are neither Send nor Sync unfortunately. Only very few types are and a big problem here is reference counting. Almost all of the types would be Send if they were not reference counted, but for a reference counted value to be Send it must also be Sync at the same time.

What's the specific problem you're trying to solve @piegamesde ? Maybe worth discussing that on IRC or the GNOME discourse in more detail?