Re: [yew-devel] [RFC PATCH] store: use try_borrow and expect_throw for better error messages

[Dominik Csapak <d.csapak@proxmox.com>]

On 5/6/25 11:09, Shannon Sterz wrote:
> On Tue May 6, 2025 at 10:49 AM CEST, Dominik Csapak wrote:
>> high level question/comment:
>>
>> AFAIU you want to add proper error messages to the borrows here,
>> so wouldn't it be enough to use 'try_borrow().expect("some error")' ?
>>
>> or does the UnrwapThrowExt trait something additional that we want/need which
>> I'm overlooking?
> 
> The `UnrwapThrowExt` is generally preferable for WASM usage according to
> the documentation:
> 

that's not what i read from the paragraph, just that it's smaller codesize.
It would only make sense to include this then if this measurably decreases
our resulting binary, maybe you can try to measure it?

In general, throwing an exception from wasm makes IMO only sense
if there is surrounding JS code that can catch the exception.

>> These methods should have a smaller code size footprint than the
>> normal `Option::unwrap` and `Option::expect` methods, but they are
>> specific to working with Wasm and JS.
> 
> Hence, why I use them here instead of the ordinary `unwrap` and `expect`
> functions.
> 
>>
>> E.g. in PDM we have a global panic hook that displays the panic (and i think stack trace)
>>
>> and the default panic hook logs it into the console
>> (which also happens for uncaught exceptions)
> 
> Yes and that is good practice, but adding these here instead of just
> panicking without any information is probably still nicer in other usage
> scenarios. So I don't really see why we shouldn't do this also.

what are 'these' and 'this' in this context? (the error messages? the expect_throw?)

I'm not opposed to add error context, on the contrary, but I question the use of
unwrapthrowext instead of normal panic'ing.

> 
> IMO, ideally we could give components a way to try acquiring the locks
> and let them handle the failure in some way. Panicking in WASM leads to
> the entire GUI freezing until it is realoaded, there may be scenarios
> where components can recover from not being able to write or read a
> store without doing so.
> 
> One scenario I can imagine is a component that uses the information in
> the Store for front-end validation. If it can't acquire a read lock on
> the store, it could fail silently and rely on the backend taking care of
> the validation. While this is somewhat inconvenient for users, it's
> probably preferable over having to reload everything (and potentially
> losing multiple inputs).
> 
> If we don't want to break the public API here, I'd suggest adding
> something like `try_read()` and `try_write()`.
> 

yeah I'd like that more than changing the existing API.
If a component knows it can recover from such an error, using a different API makes sense.

>> On 5/5/25 15:28, Shannon Sterz wrote:
>>> this makes use of the `UnwrapThrowExt` trait to improve error messages
>>> when acquring store locks. taking such locks in async functions can
>>> sometimes lead to bugs that can be unreliably reproduced, this should
>>> provide more information about the source of the error compared to
>>> just panicking.
>>>
>>> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
>>> ---
>>>
>>> alternatively we could return the error here directly (or wrap it) and
>>> let the call site handle such errors. however, that would require
>>> breaking this public api, so i decided against this for now.
>>>
>>>    src/state/store.rs | 13 +++++++++++--
>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/state/store.rs b/src/state/store.rs
>>> index bde7d9da..b071182c 100644
>>> --- a/src/state/store.rs
>>> +++ b/src/state/store.rs
>>> @@ -10,6 +10,8 @@ use yew::html::IntoEventCallback;
>>>    use yew::prelude::*;
>>>    use yew::virtual_dom::Key;
>>>
>>> +use wasm_bindgen::UnwrapThrowExt;
>>> +
>>>    use crate::props::{
>>>        ExtractKeyFn, ExtractPrimaryKey, FilterFn, IntoFilterFn, IntoSorterFn, SorterFn,
>>>    };
>>> @@ -130,7 +132,10 @@ impl<T: 'static> Store<T> {
>>>        /// Panics if the value is currently mutably locked.
>>>        pub fn read(&self) -> StoreReadGuard<T> {
>>>            StoreReadGuard {
>>> -            state: self.inner.borrow(),
>>> +            state: self
>>> +                .inner
>>> +                .try_borrow()
>>> +                .expect_throw("Could not acquire read lock on store!"),
>>>            }
>>>        }
>>>
>>> @@ -142,7 +147,11 @@ impl<T: 'static> Store<T> {
>>>        /// When the returned [StoreWriteGuard] is dropped, the store listeners
>>>        /// are notified. To prevent that use [StoreWriteGuard::skip_update]
>>>        pub fn write(&self) -> StoreWriteGuard<T> {
>>> -        let state = self.inner.borrow_mut();
>>> +        let state = self
>>> +            .inner
>>> +            .try_borrow_mut()
>>> +            .expect_throw("Could not acquire write lock on store!");
>>> +
>>>            StoreWriteGuard {
>>>                state,
>>>                update: true,
>>> --
>>> 2.39.5
>>>
>>>
>>>
>>> _______________________________________________
>>> yew-devel mailing list
>>> yew-devel@lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/yew-devel
>>>
>>>
>>
>>
>>
>> _______________________________________________
>> yew-devel mailing list
>> yew-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/yew-devel
> 



_______________________________________________
yew-devel mailing list
yew-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/yew-devel