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

Call to method on 'window' blocks when event loop in other thread is not awake #239

Closed
sagacity opened this issue Jul 20, 2017 · 9 comments
Labels
B - bug Dang, that shouldn't have happened

Comments

@sagacity
Copy link

I'm trying to construct my application in such a way that the winit (or really Glutin) event loop is on another thread, e,g.:

let events_loop = ...
let window = glutin::GlWindow::new(...pass in events_loop and windowbuilder, etc...)

thread::spawn(move || {
    events_loop.run_forever(|event| {
        ....convert events to other format and dispatch...
        ControlFlow::Continue
    });
);

This way my application is not "driven" by the winit event loop, it has just become a source of events for other parts of my code.

Anyway, the window I have is not moved to that thread, it's just the event loop that's running there. This works fine on Windows. Except on Wayland (Ubuntu) every method I invoke on window is blocked unless the event loop becomes momentarily awakened, like when I move my mouse:

// blocks!
let _ = window.get_inner_size_pixels().unwrap();

The same thing happens when calling swap_buffers on glutin's GlWindow. This will block unless the event loop is somehow awakened. Everything is fine, then.

Is this by design? Is it not supported to run the event loop on a separate thread?

@tomaka tomaka added the B - bug Dang, that shouldn't have happened label Jul 20, 2017
@elinorbgr
Copy link
Contributor

Indeed, the way the wayland backend is implemented, while the event loop is running it locks some resources exclusively, causing some methods of window to be blocking. This should obviously be improved, especially as these methods would deadlock if run from within the callback of run_forerever() ...

Now, on point that puzzles me is, looking back at the implementation, get_inner_size_pixels() is not one of the methods I would expect to see blocking, and the momentarily awakening would not have any impact on this blocking, as run_forever takes the lock until it returns.

So, this looks like your issue is something else. I'll try to reproduce your issue when I get some time in order to understand what's going on exactly. If you have a small runnable example reproducing the issue, this would be very helpful.

@sagacity
Copy link
Author

I've done some more digging, it appears that the problem shows up when trying to make the GL window current in another thread. This was not completely what I described, so apologies there! I'm not 100% sure this is allowed but I believe it should be?

The repro case is here: https://github.com/RoyJacobs/winit-repro
On my machine it blocks here: https://github.com/RoyJacobs/winit-repro/blob/master/src/main.rs#L26

If I wiggle the mouse the application continues and also calls swap_buffers without issue.

I do get this warning at startup, but it seems unrelated:

[wayland-client] Found library libwayland-client.so cannot be used: symbol wl_proxy_marshal_array_constructor_versioned

@elinorbgr
Copy link
Contributor

Oh, this warning is actually quite related. It means you have an old version of libwayland-client.so that winit cannot use, and as such it should have gone to fallback to using the X11 backend, via Xwayland.

To confirm this, can you run your example with the env variable WAYLAND_DEBUG=1 ? If you get nothing, then winit is using x11.

@sagacity
Copy link
Author

Ahh, I feel stupid for missing that! Running with that environment variable gives me identical behavior, though. It appears I'm just running x11 (sigh, I'm definitely not a Linux expert).

The questions I have then:

  • Is what I'm trying to do supported by x11 at all?
  • If I'm just running x11, why is winit trying to load a wayland client?

Thanks for your patience with this, by the way 👍

@elinorbgr
Copy link
Contributor

elinorbgr commented Jul 21, 2017

I'm no expert of X11, so I'll have to leave this to someone more knowledgeable... @Ralith maybe?

Regarding your second question: the linux ecosystem is more or less in a transition state from x11 to wayland, and during that, most wayland compositors will expose a compatibility layer named Xwayland so that X11 apps can still work with the new protocol.

To ensure max compatibility, when staed on linux, winit first tries to connect to a wayland server (as this is the most up-to-date approach), and in case of failure, fall-back to trying to connect to X11.

In your case, it looks like you have a wayland environment, but the version of the wayland libs you have is too old and missing some symbols winit needs, to use wayland, so the fallback to X11 triggers.

@sagacity
Copy link
Author

In general I'm not 100% convinced what I'm doing is legal. The reproduction scenario fails to compile on OSX since objc::runtime::Object is not Send.

I'll probably just move everything into a separate thread instead of only the event loop.

@Ralith
Copy link
Contributor

Ralith commented Jul 21, 2017

Where exactly does the call block? I didn't write the X11 Window code but I don't see any explicit locking in it, and afaik Xlib doesn't contain any that would lead to sustained blocking either.

@sagacity
Copy link
Author

sagacity commented Jul 21, 2017

It's blocking in the call to make_current. If there are no locks it could potentially be an NVidia driver issue or just some undefined behavior?

As for me, I've refactored my code to avoid threading related to the window and event loop and am now using polling in my Tokio reactor code. This works on OSX and I'll check later today if it's also solving the issue on X11.

Edit: Can confirm everything works fine on X11 when I create everything on the main thread and just poll the event loop in a separate thread.

@sagacity
Copy link
Author

I'll close this issue for now since it seems it was caused by using the library in a way that's not intended.

madsmtm added a commit to madsmtm/winit that referenced this issue Jun 11, 2022
* refactor(windows): `begin_resize_drag` now similar to gtk's (rust-windowing#200)

* refactor(windows): `begin_resize_drag` now similart to gtk's

* fix

* feat(linux): skipping taskbar will now also skip pager (rust-windowing#198)

* refactor(linux): clean dummy device_id (rust-windowing#195)

* refactor(linux): clean dummy device_id

* fmt

* feat(linux): allow resizing undecorated window using touch (rust-windowing#199)

* refactor(windows): only skip taskbar if needed when `set_visible` is called (rust-windowing#196)

* fix: increase borderless resizing inset (rust-windowing#202)

* fix: increase borderless resizing inset

* update some comments

* Replace winapi with windows crate bindings shared with WRY (rust-windowing#206)

* fix(deps): update rust crate libayatana-appindicator to 0.1.6 (rust-windowing#190)

Co-authored-by: Renovate Bot <[email protected]>

* Add Windows crate and webview2-com-sys bindings

* Initial port to webview2-com-sys

* Finish conversion and remove winapi

* Fix renamed lint warning

* Fix all match arms referencing const variables

* Put back the assert instead of expect

* Point to the published version of webview2-com-sys

* Cleanup slightly weird BOOL handling

* Replace mem::zeroed with Default::default

* Add a summary in .changes

* Remove extra projects not in config.json

* Fix clippy warnings

* Update to 32-bit compatible webview2-com-sys

* Better fix for merge conflict

* Fix clippy errors on Windows

* Use path prefix to prevent variable shadowing

* Fix Windows clippy warnings with nightly toolchain

* Fix Linux nightly/stable clippy warnings

* Fix macOS nightly/stable clippy warnings

* Put back public *mut libc::c_void for consistency

* Re-run cargo fmt

* Move call_default_window_proc to util mod

* Remove unnecessary util::to_wstring calls

* Don't repeat LRESULT expression in match arms

* Replace bitwise operations with util functions

* Cleanup more bit mask & shift with util fns

* Prefer from conversions instead of as cast

* Implement get_xbutton_wparam

* Use *mut libc::c_void for return types

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <[email protected]>

* fix(keyboard): add mapping for space key on Windows (rust-windowing#209)

* fix(keyboard): add mapping for space key on Windows

* change file

* feat: impl Clone for EventLoopWindowTarget (rust-windowing#211)

* chore: add `on_issue_closed.yml` (rust-windowing#214)

* Update tray dependency version (rust-windowing#217)

* Delete on_issue_closed.yml (rust-windowing#221)

* refactor(linux): event loop (rust-windowing#233)

* Use crossbeam::channel

* Fix crossbeam channel import

* Add check on poll event

* Fix deadlock when unregistering shortcut on Linux (rust-windowing#230)

* Add fullscreen monitor selection support on Linux (rust-windowing#235)

* Add fullscreen monitor support on Linux

* Add change file

* Remove todo on videomode

* Fix clippy

* Update to 2021 edition (rust-windowing#236)

* Update to 2021 edition

* Fix clippy

* Add run_return on Linux (rust-windowing#237)

* Add run_return on Linux

* Add main context

* Add run_return trait on Linux (rust-windowing#238)

* Fix: rust-windowing#239 Update webview2-com and windows crates (rust-windowing#240)

* Replace webivew2-com-sys with prebuilt windows

* Use windows utility instead of direct GetLastError

* Bump windows version and add changelog

* Run cargo fmt

* Restore inverted matches macro

* Scope constants in match arms

* Fix inverted null check

* Update src/platform_impl/windows/util.rs

Co-authored-by: Amr Bashir <[email protected]>

* Use env_logger instead of simple_logger (rust-windowing#241)

* Use env_logger instead of simple_logger

* Make clippy happy

* Cherry pick commits to next (rust-windowing#244)

* feat(macos): Add `unhide_application` method, closes rust-windowing#182 (rust-windowing#231)

* feat(macos): Add `unhide_application` method

* Update src/platform/macos.rs

Co-authored-by: Amr Bashir <[email protected]>

* Reanme to `show_application()`

* Remove broken doc link

Co-authored-by: Amr Bashir <[email protected]>

* feat: Allow more strings to parse to keycode (rust-windowing#229)

* feat: support accelerator key strings `,` `-` `.` `Space` `Tab` and `F13`-`F24` (rust-windowing#228)

* feat(macOS): support more accelerator key strings

* Move function keys together

* Add `,` `-` `.` `Space` `F20-F24` for Windows

* Remove support for accelerators not found in `winapi`

* Add `,` `-` `.` `Space` `F13-F24` for Linux

* Update .changes

* Add the rest for Windows

* Add the rest for Linux

* Add the rest on macOS

* Update accelerator-strings.md

* Fix git comments

Co-authored-by: Kasper <[email protected]>
Co-authored-by: Amr Bashir <[email protected]>

* Add redraw events on Linux (rust-windowing#245)

* Add redraw events on Linux

* Update doc of RequestRedraw Event

* Add change file

* Fix missing menu bar on borderless window (rust-windowing#247)

Credit goes to irh's work on winit commit f2de847

* refactor: improve `set_skip_taskbar` impl on Windows (rust-windowing#250)

* fix: emit errors on parsing an invalid accelerator for string, closes rust-windowing#135 (rust-windowing#252)

* chore: update comment

* fix(linux): fix focus events not firing properly (rust-windowing#253)

* fix(linux): fix focus events not firing properly

* add changelog

* chore: update focus events error message

* chore: fmt

* fix: revert windows-rs 0.28 version bump

* fix(linux): fix native menu items (rust-windowing#256)

* chore: remove examples commited by accident

* Update `ReceivedImeText` (rust-windowing#251)

* Allow receiving text without Ime on Windows

* Avoid panic todo

* Receive text without ime on mac

* Fix CursorMoved event on Linux

* Add ReceivedImeText on Linux

This only add Simple IME from GTK for now. We should add the actual IME
from system in the future.

* Fix redraw event that causes inifinite loop (rust-windowing#260)

* Fix redraw event that causes inifinite loop

* Refactor event loop

* Remove unused function

* Add doc comment on linux's run_return

* Ignore doc test on run_return

* Add non blocking iteration on Linux (rust-windowing#261)

* Docs: SystemTrayExtWindows::remove() is gone (rust-windowing#262)

Fix docs following rust-windowing#153

* Fix busy loop on Linux (rust-windowing#265)

* Update windows crate to 0.29.0 (rust-windowing#266)

* Update to windows 0.29.0

* Add change description

* Remove clippy check (rust-windowing#267)

* refactor(windows): align util function with win32 names

* chore: update PR template

* fix(linux): fire resized & moved events on min/maximize, closes rust-windowing#219 (rust-windowing#254)

* feat(linux): implement `raw_window_handle()` (rust-windowing#269)

* chore(deps): update to raw-window-handle 0.4

* add linux raw-window-handle support

* update macos/ios/android

* fix ios

* Fix core-video-sys dependency (rust-windowing#274)

* The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (rust-windowing#275)

However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.

* Fix some invalid msg_send! usage (rust-windowing#276)

* Revert "Fix some invalid msg_send! usage (rust-windowing#276)" (rust-windowing#277)

This reverts commit a3a2e0cfc49ddfa8cdf65cf9870fb8e3d45b4bc0.

* Revert "The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (rust-windowing#275)" (rust-windowing#279)

This reverts commit 6f9c468f26ddb60e29be2139397bfaf3b30eab1e.

* The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (rust-windowing#280)

However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly.

Co-authored-by: madsmtm <[email protected]>

* Fix some invalid msg_send! usage (rust-windowing#278)

Co-authored-by: madsmtm <[email protected]>

* Add exit code to ControlFlow::Exit (rust-windowing#281)

* Add exit code to ControlFlow::Exit

* Cargo fmt

* Add change files

Co-authored-by:  multisn8 <[email protected]>

* Add new_any_thread to Unix event loop (rust-windowing#282)

* Update windows crate to 0.30.0 (rust-windowing#283)

* Update windows crate to 0.30.0

* Simplify new-type usage

* Fix boxing in GWL_USERDATA

* Make sure everyone is using Get/SetWindowLongPtrW

* build the system_tray module when "ayatana" feature is enabled (rust-windowing#285)

Without those cfg feature checks, the "ayatana" feature does
actually not enable anything.

* Fix click events missing whe tray has menu (rust-windowing#291)

* Fix click events missing whe tray has menu

* Add change file

* Fix crash when tray has no menu (rust-windowing#294)

* chore: update pull request commit exmple

* fix(windows): send correct position for system tray events, closes rust-windowing#295 (rust-windowing#300)

* fix(windows): revert maximized state handling to winit impl, closes rust-windowing#193 (rust-windowing#299)

* fix(windows): revet maximized state handling to winit impl, closes rust-windowing#193

* add chanefile [skip ci]

* fix: `MenuItem::Quit` on Windows (rust-windowing#303)

* fix: `MenuItem::Close` on Windows

* use `PostQuitMessage` instead

Co-authored-by: amrbashir <[email protected]>

* feat: v1 audit by Radically Open Security (rust-windowing#304)

* Update to gtk 0.15 (rust-windowing#288)

* Update to gtk 0.15

* Fix picky none on set_geometry_hint

* Fix CursorMoved position

Co-authored-by: Amr Bashir <[email protected]>
Co-authored-by: Bill Avery <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Renovate Bot <[email protected]>
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
Co-authored-by: Kasper <[email protected]>
Co-authored-by: amrbashir <[email protected]>
Co-authored-by: Jay Oster <[email protected]>
Co-authored-by: madsmtm <[email protected]>
Co-authored-by: multisn8 <[email protected]>
Co-authored-by: Aurélien Jacobs <[email protected]>
Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened
Development

No branches or pull requests

4 participants