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

Make Windows main thread checks more forgiving #4003

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

PJB3005
Copy link

@PJB3005 PJB3005 commented Nov 21, 2024

The check on Windows to enforce that the event loop is created from the main thread is not fully reliable (#3999). Given that this check is not necessary on Windows and only exists more as an assert to enforce platform consistency in development, I have made it more lax:

  • It now produces a tracing::warn!() message instead of an outright panic.
  • The check is only compiled in with cfg(debug_assertions)

Fixes #3999

  • Tested on all platforms changed
  • Added an entry to the changelog module if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

The check on Windows to enforce that the event loop is created from the main thread is not fully reliable (rust-windowing#3999). Given that this check is not necessary on Windows and only exists more as an assert to enforce platform consistency in development, I have made it more lax:

* It now produces a tracing::warn!() message instead of an outright panic.
* The check is only compiled in with cfg(debug_assertions)
@PJB3005 PJB3005 requested a review from notgull as a code owner November 21, 2024 00:42
@PJB3005 PJB3005 changed the title 24 11 21 main thread Make Windows main thread checks more forgiving Nov 21, 2024
I ran cargo fmt and it just broke... oh well
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

We should only relax this check if it's being called from a DLL.

@PJB3005
Copy link
Author

PJB3005 commented Nov 21, 2024

Any idea how we'd go about checking that?

This is checked by comparing the executing module (via GetModuleHandleExA GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS) with GetModuleHandleA(NULL). If these are the same we're statically linked into the .exe.
@PJB3005
Copy link
Author

PJB3005 commented Nov 22, 2024

Alright, I figured out a way to check it, hope this is sufficient.

@notgull
Copy link
Member

notgull commented Nov 23, 2024

I would rather use a better way to detect if we are on the main thread, if possible. I've opened #4006 for this. Let me know if it works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants