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 #256

Open
SimonSapin opened this issue May 12, 2019 · 1 comment

Comments

Projects
None yet
2 participants
@SimonSapin
Copy link
Contributor

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 #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 #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: #175, #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

This comment has been minimized.

Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.