Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Figure out and document the thread-safety story #41

Open
SimonSapin opened this issue May 12, 2019 · 7 comments
Open

Figure out and document the thread-safety story #41

SimonSapin opened this issue May 12, 2019 · 7 comments

Comments

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented May 12, 2019

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 gtk-rs/cairo#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: rust-lang/rust#13231.

All of the above is not obvious, as shown by gtk-rs/cairo#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: gtk-rs/cairo#175, gtk-rs/cairo#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
Copy link
Member

@sdroege sdroege commented May 12, 2019

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
Copy link
Member

@sdroege sdroege commented Jun 19, 2019

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

  • some surfaces are Send (e.g. Image), some are not (e.g. Win32)
  • most other things in Cairo are Send
  • nothing in Cairo is Sync
  • reference counting and Send-only does not work as-is (need some kind of 0/1 reference count assurance)

@sdroege sdroege transferred this issue from gtk-rs/cairo Nov 10, 2020
@piegamesde
Copy link
Contributor

@piegamesde piegamesde commented Jan 23, 2021

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
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 23, 2021

You mean like channels? Which are also provided?

@piegamesde
Copy link
Contributor

@piegamesde piegamesde commented Jan 23, 2021

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
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 23, 2021

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

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 24, 2021

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?

elmarco referenced this issue in elmarco/gtk-rs Feb 10, 2021
Port examples builders, clock from gtk-rs/examples4
@GuillaumeGomez GuillaumeGomez transferred this issue from gtk-rs/gtk3-rs May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants