remove Sync requirement from NotificationHandler

[BillyDM]

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

[BillyDM]

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]

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]

I have also stumbled over this.

[xkr47]

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.

[wmedrano]

+1 to @xkr47. I think its fine to support jack2 and pipewire. If I remember correctly, jack1 is more single threaded, but not updated so this might just work as long as we keep supporting the current interactions.

Who knows when I'll get time for this but, roughly

1. Determine threading semantics for jack2 and pipewire.
   • Best case, its clear and/or they start documenting it.
2. Redesign jack and possibly push version 1.0!
   • Since breaking changes are a headache, its probably possible to keep maintaining the pre 1.0 API. By this I mean feature freeze, but still update dependencies.

[xkr47]

Sorry for not going into @BillyDM's original question earlier; I use crossbeam_channel instead of mpsc and that can be used in NotificationHandler even without &mut.

[crop2000]

@xkr47 do you think that the original issue is still a problem because i can use std::mpsc::senders in notification handlers directly: https://github.com/crop2000/jack_notifications_rs/blob/ea1e57ea7d2a62c36a96dd0bd01b5956ba0ba267/src/lib.rs#L128