`register_session_notification` API is not thread-safe

[jeremypenner]

The callback passed to register_session_notification is not guaranteed to run on the same thread that calls RegisterAudioSessionNotification - in fact, in practice I never observe this happening. Therefore, in order to be sound, all of the members of the EventCallbacks structs should have the + Send + Sync constraints added to their dyn Fn() types, and AudioSessionEvents should store a std::sync::Weak<EventCallbacks> instead of a std::rc::Weak<EventCallbacks>.

To demonstrate:

• Modify playsine_events.rs to prepend std::thread::current().id() to all debug!() statements.
• Observe that the setup code prints ThreadId(1), but the "New state:" line prints ThreadId(2).
• If you open the Windows volume mixer and adjust the app volume for playsine_events.exe, you can see sometimes that ThreadId(3) and ThreadId(4) is printed as well.
• Bonus weirdness: Change initialize_mta() at the start to initialize_sta(). Same behaviour! I have not been able to find any documentation that suggests that WASAPI would force free-threading behaviour on the notification object, but experimentally that seems to be what it's doing.

Annoyingly, if you make this change, your callbacks can no longer close over any structs wrapping the WASAPI COM objects. I don't actually have a lot of experience with COM, but from what I have read, I think it would ordinarily not be safe to mark them as Send or Sync, even if you can guarantee that they were created in a multi-threaded apartment. However, I'm sufficiently mystified by WASAPI's behaviour under single-threaded apartments that I can't rule out the possibility that it might actually be safe in the case of WASAPI specifically. Perhaps we could leverage AgileReference under the hood somehow?

I'm also wondering, given the apparent lack of difference between using a STA vs an MTA, if the library should just call CoIncrementMTAUsage under the hood and not expose initialize_sta() / initialize_mta() at all, as suggested by the windows-rs maintainer here: https://github.com/microsoft/windows-rs/pull/3167

[HEnquist]

Thanks for bringing this up, and the great description! I made the simple switch to std::sync::Weak here, seems to work ok but of course has the limitations you mentioned: https://github.com/HEnquist/wasapi-rs/pull/39

I'll need to read up on AgileReference and CoIncrementMTAUsage.

[jeremypenner]

Thanks for jumping on this so quickly!

While I'm here suggesting breaking changes to this interface, it might be kinda nice if the API actually had some mechanism for unregistering, instead of just leaking no-op IAudioSessionEvents objects when the callbacks struct drops. Something like:

pub fn register_session_notification(&self, callbacks: EventCallbacks) -> WasapiRes<EventRegistration> 

where EventRegistration is an opaque struct that impls Drop to unregister the callbacks?

That's obviously a more intrusive change for any existing users than just switching to Arc, but it feels a little cleaner to me. Just an idea.

[HEnquist]

That's a good way to handle it. I made a simple implementation for this in the same PR, can you take a look?