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

Re-implement MainContext channel around a manual channel #456

Merged
merged 1 commit into from Feb 19, 2019

Conversation

Projects
None yet
4 participants
@sdroege
Copy link
Member

sdroege commented Feb 18, 2019

This allows us to ensure that dropping the Receiver and its GSource will
also directly drop the closure, instead of having to wait for all
Senders to disappear too.

We have to use a mutex and a shared struct for this, and as such it
makes sense to directly implement the channel as part of this shared
struct. As the std mpsc channel internally also uses mutexes this should
not cause any considerable performance difference.

At the same time also simplify some more code and add a few more tests.

Fixes #454


CC @gdesmott this seems to work now but I'll have to write a few more tests (especially for the sync channel). Please give it a try and let me know how well it works for you. Once the Receiver or its GSource is finalized the closure should be freed now.

@gdesmott

This comment has been minimized.

Copy link

gdesmott commented Feb 18, 2019

My code isn't building with it, I'm not sure why:

error[E0277]: `*mut glib_sys::GSource` cannot be sent between threads safely
  --> src/web.rs:30:1
   |
30 | / fn play(web: State<Web>) {
31 | |     let tx = web.tx.lock().unwrap();
32 | |     tx.send(Message::Play).unwrap();
33 | | }
   | |_^ `*mut glib_sys::GSource` cannot be sent between threads safely
   |
   = help: within `glib::main_context_channel::ChannelInner<karapulse::Message>`, the trait `std::marker::Send` is not implemented for `*mut glib_sys::GSource`
   = note: required because it appears within the type `std::option::Option<*mut glib_sys::GSource>`
   = note: required because it appears within the type `glib::main_context_channel::ChannelInner<karapulse::Message>`
   = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Mutex<glib::main_context_channel::ChannelInner<karapulse::Message>>`
   = note: required because it appears within the type `(std::sync::Mutex<glib::main_context_channel::ChannelInner<karapulse::Message>>, std::option::Option<glib::main_context_channel::ChannelBound>)`
   = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<(std::sync::Mutex<glib::main_context_channel::ChannelInner<karapulse::Message>>, std::option::Option<glib::main_context_channel::ChannelBound>)>`
   = note: required because it appears within the type `glib::main_context_channel::Channel<karapulse::Message>`
   = note: required because it appears within the type `std::option::Option<glib::main_context_channel::Channel<karapulse::Message>>`
   = note: required because it appears within the type `glib::main_context_channel::Sender<karapulse::Message>`
   = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Mutex<glib::main_context_channel::Sender<karapulse::Message>>`
   = note: required because it appears within the type `web::Web`
   = note: required by `rocket::request::state::State`

Code is at https://github.com/gdesmott/karapulse/tree/cdg if you want to test.

@sdroege sdroege force-pushed the sdroege:channel-source-closure-drop branch from f2aa0dd to 3e10da6 Feb 18, 2019

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Feb 18, 2019

Oops, please try again :)

@sdroege sdroege force-pushed the sdroege:channel-source-closure-drop branch from 3e10da6 to 91f3643 Feb 18, 2019

@gdesmott

This comment has been minimized.

Copy link

gdesmott commented Feb 18, 2019

Ah, that works now. And it does to break my ref cycle as I was hoping!
Thanks a lot, you're making my life so much easier. 👍

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Feb 18, 2019

Compiles here now.

@gdesmott In this function you could use the new Bus::add_watch_local() instead of the signal to not require the closure to be Send+Sync. Maybe that helps with something?

@gdesmott

This comment has been minimized.

Copy link

gdesmott commented Feb 18, 2019

Oh, I didn't know about this API, I'll give it a shot.
Do I still have to call remove_signal_watch() once I'm done with it?

I struggled quiet a bit to get the threading/callback model working (see for example the inner pattern in Karapulse which I'm not so sure about) so any recommendation about how to do it properly is really welcome. :)

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Feb 18, 2019

Do I still have to call remove_signal_watch() once I'm done with it?

Bus::remove_watch(). That API in GStreamer is quite weird on the C level :)

I struggled quiet a bit to get the threading/callback model working (see for example the inner pattern in Karapulse which I'm not so sure about) so any recommendation about how to do it properly is really welcome. :)

Ask on IRC if you have specific questions :)

let mut inner = (self.0).0.lock().unwrap();

// If we have a bounded channel then we need to wait here until enough free space is
// available or the receiver disappears

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Feb 18, 2019

Member

You have an issue with dots, don't you? :p

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 18, 2019

Looks good to me, thanks!

Show resolved Hide resolved src/main_context_channel.rs Outdated
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Feb 18, 2019

@sdroege Thanks, 👍

Re-implement MainContext channel around a manual channel
This allows us to ensure that dropping the Receiver and its GSource will
also directly drop the closure, instead of having to wait for all
Senders to disappear too.

We have to use a mutex and a shared struct for this, and as such it
makes sense to directly implement the channel as part of this shared
struct. As the std mpsc channel internally also uses mutexes this should
not cause any considerable performance difference.

At the same time also simplify some more code and add a few more tests.

Fixes #454

@sdroege sdroege force-pushed the sdroege:channel-source-closure-drop branch from 91f3643 to 30e2756 Feb 18, 2019

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Feb 18, 2019

This should be good to go now. I've added quite a few more tests for the sync channel and various special cases, and also cleaned up some code.

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Feb 19, 2019

@EPashkin @GuillaumeGomez opinions? :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Feb 19, 2019

I already approved it and I maintain my position. :)

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Feb 19, 2019

Then let's get it in, thanks :)

@sdroege sdroege merged commit d6a3b03 into gtk-rs:master Feb 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.