Windows: Survive a OleInitialize failure. Fixes #1255

[hrydgard]

The new file drag/drop code uses OLE.

OleInitialize fails if CoInitialize(COINIT_MULTITHREADED) has previously been called on the current thread, and currently winit panics when this happens (see #1255).

There are many reasons that CoInitialize(COINIT_MULTITHREADED) might have been called before winit is initialized, for example cpal might be in use. OLE drag/drop requires COINIT_APARTMENTTHREADED but if you don't care about drag/drop, that might not be a concern.

So let's survive OleInitialize failing. Should probably warn about the lost drag/drop functionality in the log somehow - what would be the appropriate way?

• Tested on all platforms changed
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior

Not sure which documentation to update?

[hrydgard]

Turns out drag/drop is not documented in winit at all. Documenting it seems like a separate issue, so I checked off the final box here. Please go ahead and merge.

[goddessfreya]

Hey @0xpr03, I noticed you are on the testers list. Want to give this PR a test drive?

[0xpr03]

I'm back tomorrow night, then I can test this.

[hrydgard]

I added a warn! log as requested.

[0xpr03]

Tests & examples work

[hrydgard]

Just for the record, the Travis failures look unrelated? Should I just rebase this on master maybe?

[goddessfreya]

@hrydgard Yeah, a quick rebase should do it.

[goddessfreya]

Nvm, I restarted the build and everything's fine now :p

[hrydgard]

@goddessfreya it still says Changes requested by you, but everything is OK now right?

[goddessfreya]

Ah, my bad. Didn't notice @0xpr03 had tested it.

Unless @Osspial has any issues, it should be good to go? Hardly familiar with Windows, so no idea.

[Osspial]

I'm sorry for putting off responding to this for so long. I've been doing that because I don't really like the solution here, but since I haven't been able to come up with an unambiguously better solution it'll be best to air my concerns publicly so we can figure out what the best path to take will be.

I don't think this PR solves the underlying problem with managing differently-apartmented threads. Sure, it papers over the issue by suppressing the error in some situations, but I'll argue that that's significantly worse than panicking if something is wrong:

• We can't rely on downstream users having a logging system initialized to convey critical debugging information. A good number of people either aren't going to have a logging system initialized, or are going to be using a different logger (such as slog).
• The current situation, with a clear, unambiguous, show-stopping error message is much, much easier to debug than trying to figure out why drag/drop isn't working would be. I can't in good conscience force the debugging experience this PR would create on downstream users.
• This only fixes the panic in some code configurations. If you initialize cpal after Winit, you'll still run into the same error.

As such, I'm not inclined to merge this. There are certainly other ways we could expose this behavior that are less objectionable, but before we get into that I'd like to ask: why is this necessary? Cpal needs to run on its own thread to even work at all, so why do cpal's structures need to be initialized on Winit's thread?

[icefoxen]

Just ran into this myself. "clear, unambiguous, show-stopping error message" is a bit of an exaggeration. Well, "show-stopping" isn't.

[hrydgard]

I'm opposed to creating extra threads just to resolve this type of issue when it's only necessary if you really want to use OLE drag/drop. In fact, I think I'm in fact questioning the utility of OLE drag/drop in the first place - what's wrong with good old WM_DROPFILES? Actual OLE object use is pretty rare and not something that is worth sacrificing simple usage of a common library like cpal for.

And the error that happens is everything but clear and unambigious if you're not very familiar with Windows programming.

[goddessfreya]

@Osspial Out of interest, why is it that drag and drop support is dependent on COINIT_MULTITHREADED?

[hrydgard]

@goddessfreya it's the opposite, it's dependent on COINIT_APARTMENTTHREADED. And it's only so because it uses outdated OLE tech.

[Osspial]

@hrydgard Don't you have to create an extra thread anyway? Cpal's method for starting audio output takes control over the calling thread, and as such there's no way to use Winit's event loop and Cpal's event loop on the same thread at the same time.

And the error that happens is everything but clear and unambigious if you're not very familiar with Windows programming.

That's... fair. It's more accurate to say that it's relatively clear and unambiguous relatively easily diagnosable, at least compared to the alternative presented here.

In fact, I think I'm in fact questioning the utility of OLE drag/drop in the first place - what's wrong with good old WM_DROPFILES?

That's a good question! The discussion around that is in #448 - TL;DR, it's impossible to implement HoveredFile and HoveredFileCancelled without the OLE API.

[Osspial]

Does RustAudio/cpal#354 fix the underlying complaint that caused this to be an issue?

[hrydgard]

Don't know, looks like time to have another look! We worked around it before by forking cpal to use COINIT_APARTMENTTHREADED, which it already survived just fine for our use case at least.

[azriel91]

Heya @hrydgard, have you managed to find time to check?
I haven't managed to understand if the cpal fix means this one isn't necessary

[hrydgard]

We haven't yet switched to the new cpal yet, it fell down the list of priorities a little. Hope to get to it soon.

[kcking]

I can confirm this is still an issue on the latest cpal, but changing to COINIT_APARTMENTTHREADED in a fork seems to fix this issue.

[ArturKovacs]

The annoyance here is that OleInitialize calls CoInitializeEx with COINIT_APARTMENTTHREADED and according to the msdn docs there is no way around this because the ole functionality is simply not thread-safe. However, any other crate within the same program could offer a choice to call CoInitializeEx with COINIT_APARTMENTTHREADED to match Winit's behaviour. But I know that we are discussing Winit and not "any other crate" so here's my proposal.

I think there should be an option in Winit that allows to skip the ole initialization and thereby disable drag-drop and file-hover events. If this option is not set before initialization, any failure of the ole initialization triggers a panic like before. The "option" would be a crate feature or window builder parameter or something along those lines.

[kotauskas]

Any progress on this?

[Osspial]

@kotauskas #1524 implements a function that disables OLE drag-drop, which lets Winit function with COINIT_MULTITHREADED. I'm inclined to close this PR once that's merged.

[hrydgard]

Looks good. I'm happy with closing this once that's in.