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

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

[Shannon Sterz]

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

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

[Dominik Csapak]

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?

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)

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

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

[Shannon Sterz]

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:

> 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.

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()`.

> 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

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

[Dominik Csapak]

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