RustAudio / baseview

low-level window system interface for audio plugin UIs
Apache License 2.0
267 stars 57 forks source link

Don't panic when reentering wnd_proc due to mouse move or timer #130

Closed helgoboss closed 1 year ago

helgoboss commented 1 year ago

Explanation:

glowcoil commented 1 year ago

This is a similar issue to what occurs when using an external library to open a native file dialog (e.g. with the rfd crate). While obviously panicking is not ideal, simply falling back to the default WNDPROC is not really a complete solution either, since it will result in those reentrant events simply getting dropped on the floor without even being logged. As it is, at least panicking makes it obvious when this happens.

I'm also reluctant to merge this PR since it only changes some window events to use try_borrow_mut but leaves others as borrow_mut, so it's still perfectly possible to cause a panic with a reentrant event loop, it will just happen less often than timer and mouse move events.

One possible idea for a more complete solution would be to add a defer method to Window that takes a closure which will run after the current event, so that if that deferred closure results in reentrant events being delivered to the WNDPROC, the WindowHandler RefCell can be borrowed mutably to handle those events without issue.

A less involved stopgap solution would be to replace all uses of borrow_mut with try_borrow_mut, bring in the log crate, and log a warning when reentrant events are unable to be handled.

helgoboss commented 1 year ago

Thanks for the reply. Yes, I'm aware of these downsides. It was intended as a quick solution that improves one particular issue, without going "all in" ;) But of course, I would also prefer a more thorough solution.

For me, the defer approach sounds most reasonable.

helgoboss commented 1 year ago

@glowcoil I would like to work on the "defer" approach. Or are you already at it? If not, I think this defer mechanism can be something purely internal. I would check via try_borrow_mut() if it's a reentrant event and in that case enqueue the code for later execution, triggered by the timer. As queue data structure I could use VecDeque. Any objections?

glowcoil commented 1 year ago

I won't have a chance to work on this in the near future, so feel free to give it a try. However, I don't think that internally deferring events is an acceptable solution.

In the case of long-running reentrant event loops, like file dialogs, that approach will queue up events for as long as the file dialog is open and then dispatch them all at once immediately after the dialog is closed, which is definitely not desirable behavior. A secondary issue is that some functionality requires synchronously responding to events before returning from the WNDPROC, e.g. deciding whether to capture keypresses or allow them to propagate (see the EventStatus enum returned from the on_event callback), as well as proper synchronization between rendering and window resizes (see discussion here and some relevant code here), so it's not a good idea to queue up events and defer them until later for that reason.

druid-shell does have a solution involving only internal buffering, but rather than buffering events, it buffers operations which can cause events reentrantly (see DeferredOp). However, this doesn't solve the problem of external libraries which enter reentrant event loops (like rfd's synchronous API or the webbrowser crate); it only solves it for druid-shell's own built-in implementations of file dialogs and similar, and using an external library which launches a reentrant event loop from inside an event handler will still result in events being dropped on the ground.

You can think of the Window::defer method I proposed above as basically a pluggable version of druid-shell's DeferredOp which can be used for integrating arbitrary third-party libraries.

helgoboss commented 1 year ago

I'm going to open a new PR which uses the deferred_task queue added in #136.