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

Implement Sync and Send traits for all weak references. #310

Merged
merged 1 commit into from Apr 10, 2018

Conversation

Projects
None yet
3 participants
@tmiasko
Contributor

tmiasko commented Apr 9, 2018

Provide a wrapper around GWeakRef that can be safely transferred and
shared with other threads. Weak references containing types that are
Sync can be safely upgraded across different threads. In all other cases
this could potentially lead to thread safety issues. Introduce a dynamic
check that validates that upgrade operation is performed on the correct
thread.

Implement Sync and Send traits for all weak references.
Provide a wrapper around GWeakRef that can be safely transferred and
shared with other threads. Weak references containing types that are
Sync can be safely upgraded across different threads. In all other cases
this could potentially lead to thread safety issues. Introduce a dynamic
check that validates that upgrade operation is performed on the correct
thread.
@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko Apr 9, 2018

Contributor

Typical usage scenario: stash main context, and weak reference, send them to a different thread to do some work, once completed use main context to invoke closure on the original thread and upgrade the weak reference.

Contributor

tmiasko commented Apr 9, 2018

Typical usage scenario: stash main context, and weak reference, send them to a different thread to do some work, once completed use main context to invoke closure on the original thread and upgrade the weak reference.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 9, 2018

Member

Like SendCell but as weak reference? How would you ensure in your example that the object is not destroyed in the meantime, who would hold the reference? And why not use SendCell instead?

This seems to basically duplicate it and diverges from the GObject API. It should be equivalent to use a SendCell<WeakRef<T>>

Maybe WeakRef should only be Send/Sync if the contained type is?

Member

sdroege commented Apr 9, 2018

Like SendCell but as weak reference? How would you ensure in your example that the object is not destroyed in the meantime, who would hold the reference? And why not use SendCell instead?

This seems to basically duplicate it and diverges from the GObject API. It should be equivalent to use a SendCell<WeakRef<T>>

Maybe WeakRef should only be Send/Sync if the contained type is?

@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko Apr 9, 2018

Contributor

How would you ensure in your example that the object is not destroyed in the meantime, who would hold the reference?

The point of using weak references is that the object can be destroyed in the
meantime. In this case upgrade method just returns None, in the same way other
weak pointers in Rust do.

And why not use SendCell instead?

Because SendCell has to be either unsafe or leak memory?

This seems to basically duplicate it and diverges from the GObject API.

It has two additional features: upgrades are thread-safe, and weak pointer can
be safely cloned. It also retains compatibility with GObject API in principle.
I didn't implement usual conversion routines because weak pointers aren't
generally used in public API, so I don't have any need for them.

Maybe WeakRef should only be Send/Sync if the contained type is?

IMHO the primary uses case are things that aren't Sync, like GtkWidgets where
you need to prepare data before updating widget. If widget is destroyed in the
meantime that is perfectly fine, you just don't update it. Not to mention that
there are only a handful of things that are Sync and are Objects, and for them
you may not need weak references in the first place.

Contributor

tmiasko commented Apr 9, 2018

How would you ensure in your example that the object is not destroyed in the meantime, who would hold the reference?

The point of using weak references is that the object can be destroyed in the
meantime. In this case upgrade method just returns None, in the same way other
weak pointers in Rust do.

And why not use SendCell instead?

Because SendCell has to be either unsafe or leak memory?

This seems to basically duplicate it and diverges from the GObject API.

It has two additional features: upgrades are thread-safe, and weak pointer can
be safely cloned. It also retains compatibility with GObject API in principle.
I didn't implement usual conversion routines because weak pointers aren't
generally used in public API, so I don't have any need for them.

Maybe WeakRef should only be Send/Sync if the contained type is?

IMHO the primary uses case are things that aren't Sync, like GtkWidgets where
you need to prepare data before updating widget. If widget is destroyed in the
meantime that is perfectly fine, you just don't update it. Not to mention that
there are only a handful of things that are Sync and are Objects, and for them
you may not need weak references in the first place.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 9, 2018

Member

Sorry if my comment seemed harsh, I wrote it in a hurry on the phone.

Your changes look useful, and the thing I missed why SendCell can't be used is that SendCell<WeakRef<T>> would leak/be unsafe because it would potentially be destroyed on another thread, but it actually is fine to destroy the WeakRef from another thread. Correct?

Member

sdroege commented Apr 9, 2018

Sorry if my comment seemed harsh, I wrote it in a hurry on the phone.

Your changes look useful, and the thing I missed why SendCell can't be used is that SendCell<WeakRef<T>> would leak/be unsafe because it would potentially be destroyed on another thread, but it actually is fine to destroy the WeakRef from another thread. Correct?

@sdroege

generally looks good to me, maybe could get a test :)

let c = WeakRef(Box::new(mem::uninitialized()), PhantomData);
/// An error returned by [`WeakRef::try_upgrade`](struct.WeakRef.html#method.try_upgrade).
#[derive(Debug)]
pub struct UpgradeError {

This comment has been minimized.

@sdroege

sdroege Apr 9, 2018

Member

We usually just use BoolError for such things here

@sdroege

sdroege Apr 9, 2018

Member

We usually just use BoolError for such things here

This comment has been minimized.

@tmiasko

tmiasko Apr 9, 2018

Contributor

I was thinking about adding additional debug information like thread IDs, that
is why I used separate error type and left a room to change implementation
details without breaking API. Though, neither GLib thread IDs, nor Rust IDs
correspond to native threads IDs you could easily see in debugger, which I
think would be most useful. Thus for now I left error message without
additional details.

@tmiasko

tmiasko Apr 9, 2018

Contributor

I was thinking about adding additional debug information like thread IDs, that
is why I used separate error type and left a room to change implementation
details without breaking API. Though, neither GLib thread IDs, nor Rust IDs
correspond to native threads IDs you could easily see in debugger, which I
think would be most useful. Thus for now I left error message without
additional details.

@tmiasko

This comment has been minimized.

Show comment
Hide comment
@tmiasko

tmiasko Apr 9, 2018

Contributor

Yes, WeakRef can be safely destroyed in any thread, because it doesn't own the object referenced.

Contributor

tmiasko commented Apr 9, 2018

Yes, WeakRef can be safely destroyed in any thread, because it doesn't own the object referenced.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 9, 2018

Member

Yes, WeakRef can be safely destroyed in any thread, because it doesn't own the object referenced.

Yeah, agreed. And because WeakRef handling in GObject actually is thread-safe.

Member

sdroege commented Apr 9, 2018

Yes, WeakRef can be safely destroyed in any thread, because it doesn't own the object referenced.

Yeah, agreed. And because WeakRef handling in GObject actually is thread-safe.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 10, 2018

Member

@GuillaumeGomez @EPashkin Any opinions? IMHO good to go

Member

sdroege commented Apr 10, 2018

@GuillaumeGomez @EPashkin Any opinions? IMHO good to go

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 10, 2018

Member

My expertise in this area is very close to nothing so if it seems good to you, then it's good for me. :)

Member

GuillaumeGomez commented Apr 10, 2018

My expertise in this area is very close to nothing so if it seems good to you, then it's good for me. :)

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Apr 10, 2018

Member

Go for it then, if you like the new type for the error

Member

sdroege commented Apr 10, 2018

Go for it then, if you like the new type for the error

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Apr 10, 2018

Member

Then let's go!

Member

GuillaumeGomez commented Apr 10, 2018

Then let's go!

@GuillaumeGomez GuillaumeGomez merged commit 88ada82 into gtk-rs:master Apr 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

This makes usage of weak references quite annoying.

Whenever having a weak reference you now need to think whether you call downgrade() or downgrade_sync(). Both will always work, but one of them will panic if upgrade() is called from a different thread.

Previously sending a weak ref from downgrade() to another thread would simply be a compiler error if the type was not sync.

I think we should find another solution here. IMHO we should revert this PR and instead provide a SendWeakRef wrapper that solves this by behaving like SendCell but not panic in drop?

@tmiasko @GuillaumeGomez opinions?

Member

sdroege commented Jul 25, 2018

This makes usage of weak references quite annoying.

Whenever having a weak reference you now need to think whether you call downgrade() or downgrade_sync(). Both will always work, but one of them will panic if upgrade() is called from a different thread.

Previously sending a weak ref from downgrade() to another thread would simply be a compiler error if the type was not sync.

I think we should find another solution here. IMHO we should revert this PR and instead provide a SendWeakRef wrapper that solves this by behaving like SendCell but not panic in drop?

@tmiasko @GuillaumeGomez opinions?

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 25, 2018

Member

If it requires more thoughts on user's side, then it's not good and it means the binding should be improved instead. I'm totally in favor of another solution.

Member

GuillaumeGomez commented Jul 25, 2018

If it requires more thoughts on user's side, then it's not good and it means the binding should be improved instead. I'm totally in favor of another solution.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 25, 2018

Member

It does things implicitly that lead to panics now and need users to think about it all the time when using the API.

My proposed solution would give a compiler error and users need to explicitly opt-in for the panic-behaviour if they want it.

EDIT: Also unrelated, SyncObjectExt is not even re-exported by the prelude or anything right now from glib, making usage even more awkward.

Member

sdroege commented Jul 25, 2018

It does things implicitly that lead to panics now and need users to think about it all the time when using the API.

My proposed solution would give a compiler error and users need to explicitly opt-in for the panic-behaviour if they want it.

EDIT: Also unrelated, SyncObjectExt is not even re-exported by the prelude or anything right now from glib, making usage even more awkward.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 25, 2018

Member

I really prefer this.

Member

GuillaumeGomez commented Jul 25, 2018

I really prefer this.

sdroege added a commit to sdroege/glib-rs that referenced this pull request Jul 25, 2018

sdroege added a commit to sdroege/glib-rs that referenced this pull request Jul 25, 2018

Add new SendWeakRef type
This allows sending weak references to arbitrary threads, to clone and
drop them from arbitrary threads but will panic when trying to upgrade
it from a different thread than where it was created on.

It works for all object types, while sending normal WeakRefs to
different threads only works if the object itself is Sync and Send.

See gtk-rs#310 (comment)

@sdroege sdroege referenced this pull request Jul 25, 2018

Merged

Add new SendWeakRef type #359

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment