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

remove Sync requirement from NotificationHandler #167

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BillyDM
Copy link

@BillyDM BillyDM commented Feb 19, 2022

The Sync requirement on NotificationHandler is unecessary, and it is preventing me from storing an mpsc::Sender in my struct.

@BillyDM
Copy link
Author

BillyDM commented Feb 19, 2022

Also, I made self in NotificationHandler::thread_init mutable so I can actually use my mpsc::Sender to send a message to my GUI.

@wmedrano
Copy link
Member

I'll have to double check that this is valid. If this is not valid, then you will probably have to use a Mutex and/or SyncSender.

@Be-ing
Copy link
Contributor

Be-ing commented Feb 22, 2022

I have also stumbled over this.

@xkr47
Copy link

xkr47 commented Aug 13, 2023

Also, I made self in NotificationHandler::thread_init mutable so I can actually use my mpsc::Sender to send a message to my GUI.

Due to #104 I made a small test function:

    fn thread_init(&self, _: &Client) {
        info!("thread_init enter {:?}", thread::current().id());
        thread::sleep(Duration::from_secs(1));
        info!("thread_init exit {:?}", thread::current().id());
    }

..which gives the following output:

2023-08-13 21:12:56.358413 INFO [jack_connector::my_jack] thread_init enter ThreadId(6)
2023-08-13 21:12:57.358612 INFO [jack_connector::my_jack] thread_init exit ThreadId(6)
2023-08-13 21:12:57.358849 INFO [jack_connector::my_jack] thread_init enter ThreadId(7)
2023-08-13 21:12:57.368143 INFO [jack_connector::my_jack] thread_init enter ThreadId(8)
2023-08-13 21:12:58.359022 INFO [jack_connector::my_jack] thread_init exit ThreadId(7)
2023-08-13 21:12:58.368318 INFO [jack_connector::my_jack] thread_init exit ThreadId(8)

.. which reveals two threads (7 and 8) to be running thread_init() simultaneously. Therefore giving mutable access to self will yield undefined behaviour.

The Sync requirement on NotificationHandler is unecessary, and it is preventing me from storing an mpsc::Sender in my struct.

I think the #121 findings should be investigated thoroughly before concluding that only a single method of the NotificationHandler trait will ever be running at any given time, despite being called in different threads.

The findings also makes me wonder whether it is safe to have &mut self passed to the other NotificationHandler callbacks than thread_init..

And in general I think the point that jack has 2 (or is pipewire a 3rd?) implementations also needs to be considered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants