[codex] Implement simplified flame-rs API#456
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a higher-level Rust SDK API for Flame, significantly reducing boilerplate for both clients and services. Key features include the FlameMessage trait for typed payload serialization, ergonomic client helpers like SessionOptions and TaskHandle, and procedural macros (#[entrypoint] and #[instance]) for defining services. The flmping and flmexec components have been refactored to utilize these new APIs. Review feedback identifies a blocking std::thread::sleep call in an asynchronous context, inconsistencies in error mapping within the macro implementation compared to the design document, and the use of unwrap() in documentation examples which contradicts the project's reliability goals.
|
|
||
| let mem_size = Byte::from_u64(mem.len() as u64).to_string(); | ||
| if let Some(duration) = input.duration { | ||
| thread::sleep(Duration::from_millis(duration)); |
There was a problem hiding this comment.
Using std::thread::sleep in an async function is problematic as it blocks the executor's thread, preventing other tasks from running. This can lead to performance issues and deadlocks in a concurrent environment. You should use the asynchronous version from tokio: ::tokio::time::sleep(...).await.
| thread::sleep(Duration::from_millis(duration)); | |
| ::tokio::time::sleep(Duration::from_millis(duration)).await; |
| #[flame_rs::instance] | ||
| impl ModelService { | ||
| async fn enter(&self, _instance: FlameInstance) -> Result<(), FlameError> { | ||
| *self.assets.lock().unwrap() = Some(load_assets()?); |
There was a problem hiding this comment.
The example code here and in the following lines (513, 514, 518) uses .unwrap() on Mutex lock results. This can cause the service to panic if the lock is poisoned. This contradicts the design's own reliability goal on line 1022, which states that "Generated code must avoid unwrap." The generated code sketch starting at line 872 correctly handles poisoned locks with map_err. It would be best to update this example to reflect the same robust error handling to provide a consistent and safe implementation pattern.
| impl #impl_generics #crate_path::message::FlameMessage for #name #ty_generics #where_clause { | ||
| fn encode( | ||
| &self, | ||
| ) -> ::std::result::Result<#crate_path::__private::bytes::Bytes, #crate_path::apis::FlameError> { | ||
| #crate_path::__private::serde_json::to_vec(self) | ||
| .map(#crate_path::__private::bytes::Bytes::from) | ||
| .map_err(|err| #crate_path::apis::FlameError::InvalidConfig(err.to_string())) | ||
| } | ||
|
|
||
| fn decode(bytes: &[u8]) -> ::std::result::Result<Self, #crate_path::apis::FlameError> { | ||
| #crate_path::__private::serde_json::from_slice(bytes) | ||
| .map_err(|err| #crate_path::apis::FlameError::Internal(err.to_string())) | ||
| } | ||
| } |
There was a problem hiding this comment.
The error mapping for encode and decode seems to be swapped compared to the design document and general error handling principles.
encode: An error during encoding a service's response should be anInternalerror. The current implementation maps it toInvalidConfig.decode: An error when decoding a client's request should be anInvalidConfigerror. The current implementation maps it toInternal.
This is inconsistent with the design document (RFE455), which specifies on lines 990 and 997 that decode failures should be InvalidConfig and encode failures should be Internal.
| impl #impl_generics #crate_path::message::FlameMessage for #name #ty_generics #where_clause { | |
| fn encode( | |
| &self, | |
| ) -> ::std::result::Result<#crate_path::__private::bytes::Bytes, #crate_path::apis::FlameError> { | |
| #crate_path::__private::serde_json::to_vec(self) | |
| .map(#crate_path::__private::bytes::Bytes::from) | |
| .map_err(|err| #crate_path::apis::FlameError::InvalidConfig(err.to_string())) | |
| } | |
| fn decode(bytes: &[u8]) -> ::std::result::Result<Self, #crate_path::apis::FlameError> { | |
| #crate_path::__private::serde_json::from_slice(bytes) | |
| .map_err(|err| #crate_path::apis::FlameError::Internal(err.to_string())) | |
| } | |
| } | |
| impl #impl_generics #crate_path::message::FlameMessage for #name #ty_generics #where_clause { | |
| fn encode( | |
| &self, | |
| ) -> ::std::result::Result<#crate_path::__private::bytes::Bytes, #crate_path::apis::FlameError> { | |
| #crate_path::__private::serde_json::to_vec(self) | |
| .map(#crate_path::__private::bytes::Bytes::from) | |
| .map_err(|err| #crate_path::apis::FlameError::Internal(err.to_string())) | |
| } | |
| fn decode(bytes: &[u8]) -> ::std::result::Result<Self, #crate_path::apis::FlameError> { | |
| #crate_path::__private::serde_json::from_slice(bytes) | |
| .map_err(|err| #crate_path::apis::FlameError::InvalidConfig(err.to_string())) | |
| } | |
| } |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Summary
flame-rsclient APIFlameMessage, typed task/common-data helpers,SessionOptions,Session::invoke,Session::run, andTaskHandleflame-rs-macrossupport for#[derive(FlameMessage)],#[entrypoint], and#[instance]flmpingto use the typed macro path and updateflmexecto use the new session/root runner helpersValidation
cargo fmtcargo check -p flame-rscargo check -p flame-rs --features macroscargo check -p flmpingcargo check -p flmexeccargo test -p flame-rs --lib --features macroscargo test -p flame-rs --features macros --test rfe455_macro_testcargo metadata --no-deps --format-version 1git diff --check