You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Maybe this could be added to this crate.
But even if it's out of scope, I thought it's kind of interesting and wanted to share.
Problem
Since window_proc() function of win_win::WindowProc trait takes &self, mutable state has to be wrapped in RefCell<>.
In window_proc() we use .borrow_mut() to access it.
However, if we forget to release this borrow when calling Windows API functions that cause window proc reentrancy (e.g. SendMessage() or MessageBox()), second invocation of window_proc() will fail with a runtime error "already borrowed".
This failure to account for reentrancy could be easily missed by manual testing if .borrow_mut() only happens on some paths in window_proc().
It would be nice to have some kind of compile-time enforcement.
Solution
Instead of passing RefCell<State> to window_proc(), we will pass some opaque type StateRef<State> with the following public interface:
fnwindow_proc(sr:&mutStateRef<State>,hwnd:HWND,msg:UINT,_wparam:WPARAM,_lparam:LPARAM,)-> Option<LRESULT>{letmut state = sr.state_mut();// do stuff with stateif msg == WM_LBUTTONDOWN{drop(state);// Release state borrow.// Borrow checker will compain if we forget to do it.message_box(sr.reent(), hwnd,"hello","title",MB_OK);letmut state = sr.state_mut();// borrow it again if needed// do stuff}None}
That's quite fascinating. I do see the argument and think it is sound. I'm not sure it can readily be adapted to something like druid, where I think it would be quite difficult to drop the borrow to the window state from, eg, inside a widget handling method. Another major concern is that you have to catch all entry points that can cause reentrancy, while the RefCell approach does guarantee coverage of all cases where you get reentrancy.
I'll point to linebender/druid#1068 as an example that we're still running into serious reentrancy issues.
Thanks for sharing this! If other people would find it useful, I can see adding it. (If it's a parametrized type, possibly it can be done in a way that would cause minimal impact if not actually used).
you have to catch all entry points that can cause reentrancy, while the RefCell approach does guarantee coverage of all cases where you get reentrancy
My approach is an additional layer on top of the runtime protection RefCell provides. I'm not suggesting to replace RefCell with UnsafeCell or anything (that would indeed require exhaustively annotating all winapi calls that might cause reentrancy).
The impl DerefMut<Target=State> that .state_mut() returns is actually std::cell::RefMut<State>.
If you forget to annotate message_box() as taking Reent, the compiler will allow you to call it without releasing state borrow, but it will panic because of the RefCell check.
Maybe this could be added to this crate.
But even if it's out of scope, I thought it's kind of interesting and wanted to share.
Problem
Since
window_proc()
function ofwin_win::WindowProc
trait takes&self
, mutable state has to be wrapped inRefCell<>
.In
window_proc()
we use.borrow_mut()
to access it.However, if we forget to release this borrow when calling Windows API functions that cause window proc reentrancy (e.g.
SendMessage()
orMessageBox()
), second invocation ofwindow_proc()
will fail with a runtime error "already borrowed".This failure to account for reentrancy could be easily missed by manual testing if
.borrow_mut()
only happens on some paths inwindow_proc()
.It would be nice to have some kind of compile-time enforcement.
Solution
Instead of passing
RefCell<State>
towindow_proc()
, we will pass some opaque typeStateRef<State>
with the following public interface:Reent
is a zero-sized type that proves that we are not holding mut borrow ofState
at the moment.Windows API calls that are known to cause reentrancy should have safe wrappers that take a
Reent
argument:Usage example:
Complete working example: https://github.com/Vlad-Shcherbina/win-win-reent-demo
The text was updated successfully, but these errors were encountered: