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

fix: Windows hangs when closing midi input #151

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

j-n-f
Copy link
Contributor

@j-n-f j-n-f commented Apr 10, 2024

  • Mutexes in Windows are re-entrant, but std::sync::Mutex is not.
  • If a sysex message is inbound when closing a MIDI input windows will hang because part of the call to midiInReset will cause the input handler to fire, but the HMIDIIN lock was already taken before that handler attempts to take the lock a second time.
  • All of this takes place in a single thread, so there's no risk in allowing the lock to be taken twice.
  • parking_lot::ReentrantMutex is used in place of Mutex.
  • This change should behave more like rtmidi (which some of this code is based on) as rtmidi is also using a re-entrant mutex.
  • Tested on Windows 10, and issue appears resolved.

Fixes #150

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 10, 2024

@Boddlnagg does this look like an intermittent failure? https://dev.azure.com/Boddlnagg/midir/_build/results?buildId=290&view=logs&j=2ae84081-e78e-5a7a-8f41-ad37cdcf10e4&t=b918b31c-3069-5944-d0e6-943aa58d75cf

I don't think any of these changes would cause an issue with the linker, but maybe I'm wrong.

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 10, 2024

actually, I may need to do some more debugging. I think the build failure is a fluke, but it looks like I'm still seeing the same bug.

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 10, 2024

all right, sorry for the confusion. The fix is verified as working.

I made a mistake in my Cargo.toml when I added [patch.crates-io] to test changes locally against my application (I was adding it to the Cargo.toml in a workspace folder, and not at the root).

As an aside, this crate has made my life a lot easier and I just wanted to say thank you for making and maintaining it.

Copy link
Owner

@Boddlnagg Boddlnagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for figuring this out and providing a fix!

Cargo.toml Show resolved Hide resolved
@Boddlnagg
Copy link
Owner

Boddlnagg commented Apr 20, 2024

Do you have any idea why the i686-w64-mingw32-gcc build fails in CI? Could this be related to the change or just spurious? It looks like parking_lot might not be compatible with mingw32 ...

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 20, 2024

Do you have any idea why the i686-w64-mingw32-gcc build fails in CI? Could this be related to the change or just spurious? It looks like parking_lot might not be compatible with mingw32 ...

I'm guessing it's just spurious, but I don't know how to re-run the pipeline. Do you have any instructions on how to re-run a pipeline that's spuriously failing?

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 20, 2024

It looks like it builds parking_lot fine here: https://dev.azure.com/Boddlnagg/midir/_build/results?buildId=290&view=logs&j=2ae84081-e78e-5a7a-8f41-ad37cdcf10e4&t=26f8685c-4e6b-5527-1b2b-b6154ff0fd7a

Here's what I'm trying to reproduce the failure:

  1. rustup toolchain install stable-i686-pc-windows-gnu (installed rustc 1.77.2 (25ef9e3d8 2024-04-09))
  2. rustup run stable-i686-pc-windows-gnu cargo build (builds fine)
  3. rustup run stable-i686-pc-windows-gnu cargo build --example test_list_ports (see the same build failure)

This is unfortunate. I don't see a good solution here. The most common compiler/linker for windows (MSVC) works fine. I'm not sure what's unique about parking_lot that doesn't allow the build to work for mingw.

We could make the re-entrant mutex conditional on using MSVC, but now mingw will build but the code will behave incorrectly. parking_lot doesn't have any issues related to mingw.

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 20, 2024

potential fix: it seems like mingw issue might be fixed by dynamically linking to windows SDK: ardaku/whoami@8c673fe

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 20, 2024

Interesting, it looks like updating windows from 0.43 to 0.56 resolves the issue.

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 20, 2024

all right, now it seems like the tradeoff is between supporting mingw and updating midir to use a newer version of the windows crate (winRT builds are failing in a non-obvious way now). I'm going to try seeing if there's a version of windows that makes all builds happy without requiring me to do a bunch of extra unrelated work.

* Mutexes in Windows are re-entrant, but `std::sync::Mutex` is not.
* If a sysex message is inbound when closing a MIDI input windows will
  hang because part of the call to `midiInReset` will cause the input
  handler to fire, but the `HMIDIIN` lock was already taken before that
  handler attempts to take the lock a second time.
* All of this takes place in a single thread, so there's no risk in
  allowing the lock to be taken twice.
* `parking_lot::ReentrantMutex` is used in place of `Mutex`.
* This change should behave more like `rtmidi` (which some of this code
  is based on) as `rtmidi` is also using a re-entrant mutex.
* Tested on Windows 10, and issue appears resolved.
* Update `windows` from `0.43` to `0.56` to resolve mingw-specific
  issues.
* Minor changes to `winrt` code to accomodate update to `windows`
  crate.

Fixes Boddlnagg#150
Fixes Boddlnagg#152
@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 20, 2024

@Boddlnagg we are good to go. 😀

@Boddlnagg Boddlnagg merged commit 52dfd88 into Boddlnagg:master Apr 21, 2024
10 checks passed
@Boddlnagg Boddlnagg mentioned this pull request Apr 21, 2024
@Boddlnagg
Copy link
Owner

Thanks again! I just released version 0.10.0 with this fix (and other changes that had accumulated).

@j-n-f
Copy link
Contributor Author

j-n-f commented Apr 21, 2024

Right on. The timely release is much appreciated :)

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.

[Windows] midi input locks up if you close while receiving data
2 participants