wasapi: `Stream` no longer `Sync`

[mnmaita]

I was working on updating rodio and cpal on the bevy crate and I've encountered a couple of build issues for different targets.

I've tested patching cpal by using the following SHA: a8269d3 and the build issues we had went away for MacOS targets. But now, this change seems to have made Stream non-Sync and this breaks a Resource (which need to be both Send and Sync as far as I'm aware) wrapping the audio stream.

The field that seems to be the causing this issue is the event field which has the type struct HANDLE(pub *mut core::ffi::c_void), i.e. Foundation::HANDLE from the windows crate, which doesn't seem to be neither Send nor Sync. I couldn't find how these trait guarantees were upheld before, as a very similar type was being used for this field (https://docs.rs/winapi/latest/winapi/um/winnt/type.HANDLE.html).

I hope this issue has enough detail, and if not please let me know. I might be overlooking something here so any feedback and insights will be welcomed. Thanks a lot for your time!

[roderickvd]

The change you linked to is from 2022, while the latest master should guarantee Stream to implement Sync on all hosts. It’s not been released yet.

[roderickvd]

Sorry, scrap that: it guarantees Send, not Sync. I think that generally that will be difficult to uphold and be safe for streams.

Previously, some of the streams had Sync implemented unsafely and, in fact, be unsafe, and had that removed.

[roderickvd]

Sorry for the train of thought, but as I got some time to think about it:

I think WASAPI could implement Sync as-is, because:

1. JoinHandle is Sync
2. Sender is Sync
3. pending_scheduled_event is a Windows event handle, and signaling events is thread-safe
4. All COM interface operations happen on the dedicated audio thread

…however, it would be inconsistent when other hosts do not and could not implement Sync (without locking everything behind an Arc<Mutex<_>>), so that’d not be very cross-platform.

Is there any swaying argument why it should be Sync on some but not all hosts?

[mnmaita]

If I understand correctly, the inconsistency is there right now. With coreaudio the Stream seems to comply with Send + Sync bounds but the same can't be said for the Windows API after the change I've mentioned earlier. If you could check bevyengine/bevy#20323 it might give you some more context regarding the issue I'm facing with this update and maybe you will have an idea to move forward. Thanks a ton for your insights!

[IamLuisAUT]

I encounter the same issue.

In my case, however, the commit breaking my build and removing the Sync trait from the WASAPI Stream in cpal is 87b7036. When using the parent commit or v0.16.0 published on crates.io, the WASAPI Stream is Sync.

It seems as if the upgrade of windows to 0.62 removed Sync from pending_scheduled_event: windows::Win32::Foundation::HANDLE. Unfortunately, I lack the experience and knowledge required to justify whether the handle should be safe to share between threads or to provide a pull request.

As @mnmaita already mentioned, the current CoreAudio (macOS) and ALSA Stream implementation on main is already Sync. Removing Sync from the WASAPI Stream in a new release will potentially break other applications too, and in my honest opinion, it would be more cross-platform if the WASAPI Stream was Sync. I did not check for any other host/platform implementation.

I am happy to provide more context if necessary.

[roderickvd]

Please review #1045 where Stream is now Sync on all hosts.

[mnmaita]

@roderickvd thanks for addressing this! I will try to find some time soon to test it in my bevy update branch. Wondering when we can expect a release containing this change.

[roderickvd]

Soon. Need to get #1046 down.

[mnmaita]

Soon. Need to get #1046 down.

First of all, thanks for all the work you are doing on this crate! Second, I saw that PR you mention was merged a couple weeks ago so I wonder if there's an ETA for a new release now? Thanks again 🙏🏻

[roderickvd]

Thank you.

See #1065 for v0.17 release: do help test for these last days. If nothing major pops up, maybe this weekend.
#1070 would be nice for Bevy too I guess, awaiting feedback.