[MacOS] Fix deadlock on maximizing window from event callback #2636
Conversation
|
potentially reopens #1837 |
|
Could you elaborate where deadlock happens? Is it some winit structure preventing it or it's deeper in macOS libs? It just if it happens from the event callback it would mean that other calls are also affected by that? Maybe we can simply copy some state around to release the locks? |
|
What is happening is that Here's a full callstack for the stuck situation from the linked egui pr, patched with latest winit: |
|
so maybe we could also fix this by skipping over in pub fn handle_redraw(window_id: WindowId) {
if !HANDLER.get_in_callback() {
HANDLER.handle_nonuser_event(EventWrapper::StaticEvent(Event::RedrawRequested(window_id)));
}
}but it didn't quite check out for me why queue redraw isn't the more correct thing here? EDIT: Just checked, this would be a sufficient fix if you prefer that :) |
|
another venue would be pushing |
|
The more I read into the code, the less it makes sense to me. Winit has a clear policy when So it means that @madsmtm could you elaborate why macOS has things that way? That just looks wrong compared to 1. our docs. 2. other backends. So at first I'd like to figure out, why the code is written the way it is, so we can continue with more clear solution other that hot-fixing. |
|
To clarify. The 2 sources I was talking about is
The 2-nd looks like a canonical and complies with the winit's docs. The 1-st is what looks out of place to me. |
|
Some previous discussion on this:
I agree that we should just be able to set a flag, and then handle it after I think I prefer the approach described in #2636 (comment) for now, which fixes the immediate issue as well as keeping roughly the desired behaviour (instant resize). Though yes, the events would still appear of order, I'll try to fix this properly at some point (there are ways to do this properly, they just need more research). |
|
Maybe we need a separate event telling that The issue with new event that it'll basically be utilized only on macOS... If you're fine with current approach, I'd suggest to document that the |
Wumpf
force-pushed
the
fix-deadlock-on-maximize-from-callback
branch
from
5f90efb to
67b5a25
Compare
|
Changed this PR over to the hotfix meanwhile. |
Wumpf
force-pushed
the
fix-deadlock-on-maximize-from-callback
branch
from
67b5a25 to
fedc4e6
Compare
Wumpf
force-pushed
the
fix-deadlock-on-maximize-from-callback
branch
from
fedc4e6 to
bfea199
Compare
Wumpf
force-pushed
the
fix-deadlock-on-maximize-from-callback
branch
from
bfea199 to
f76fa73
Compare
Wumpf
requested review from
kchibisov
and removed request for
madsmtm and
kchibisov
|
Thanks, I went ahead and merged this, and since I didn't have access to edit your branch, opened #2641 for the documentation change as suggested by @kchibisov. |
This sadly undos #1901 again - cc: @xiaopengli89
The problem is that
handle_redrawhas no threading/re-entrance protection whatsoever - when callingset_maximizedfrom within an event it will deadlock (which is ofc much worse than what #1901 tried to address)!I ran into this when testing this egui PR emilk/egui#2292
CHANGELOG.mdif knowledge of this change could be valuable to users