AA: kbs: Improve handling of invalid RCAR JSON #723
Conversation
|
This PR addresses @mkulke's comment: #712 (comment). |
jodh-intel
force-pushed
the
aa-kbs-rcar-improvement
branch
from
934118a to
a0e4f74
Compare
| @@ -56,6 +56,26 @@ async fn get_request_extra_params() -> serde_json::Value { | |||
| extra_params | |||
| } | |||
|
|
|||
| async fn get_hash_algorithm(extra_params: serde_json::Value) -> Result<HashAlgorithm> { | |||
There was a problem hiding this comment.
we might not need this to be async
There was a problem hiding this comment.
@Xynnn007 - true, this function doesn't actually need to be async, but I feel the code is cleaner + safer if we use async everywhere we can reasonably do so.
If I drop the async, I either:
- block the calling task implicitly (maybe not an issue now, but potentially problematic if the implementation changes (future bug potential)).
- block it explicitly with
spawn_blocking()(extra code noise for no obvious benefit).
There was a problem hiding this comment.
I'm not sure that coloring everything in the call tree async is a good approach. Can you elaborate on the safety risk? for all I know the async annotation functions should be reserved to functions that do await a Future.
tokio is a work-stealing scheduler, even a large sync computation wouldn't block an eventloop or something.
the annotation of a fn with async desugars into a future return type in the fn signature and makes things more complicated in terms of lifetimes (futures need to be sync + send, as they cannot be shared across threads), you have to start an executor for a unit test, etc...
so, intuitively I'd say that async should be strictly limited to places where it's absolutely required, but I'm happy for pointers to docs/articles that say otherwise.
There was a problem hiding this comment.
My personal preference is to mark it as non-async. Once it needs to be async, we can then mark it as async version.
btw, if a function is a concrete implementation of an async-trait function, I can live with situations where even no async code is inside the function body.
wdyt?
There was a problem hiding this comment.
I think we've spent enough time discussing a 5 byte change. Branch updated 😄
Implement `PartialEq` for `HashAlgorithm` to allow two hash algorith values to be compared. Signed-off-by: James O. D. Hunt <[email protected]>
Create a new error to handle the scenario where the RCAR JSON specifies the selected hash algorithm using the wrong data type (any type other than 'string'). Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel
force-pushed
the
aa-kbs-rcar-improvement
branch
from
a0e4f74 to
6c30f4b
Compare
Create a new error to handle the scenario where the RCAR JSON specifies the selected hash algorithm
using the wrong data type (any type other than 'string').
Signed-off-by: James O. D. Hunt [email protected]