fix[simulation] :: prevent infinite loops via input validation#15
Conversation
Summary of ChangesHello @bniladridas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the stability and resource management of the simulation engine. It introduces essential input validation for simulation parameters to prevent issues like infinite loops caused by invalid inputs. Furthermore, it implements a mechanism to limit the number of concurrent simulations, mitigating the risk of resource exhaustion, and enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
bniladridas
changed the title
bniladridas
changed the title
![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 important input validation and resource limits for the simulation engine. The changes correctly prevent simulations from starting with invalid parameters and cap the number of concurrent simulations. My review focuses on improving the API design to provide feedback to the caller on failure, and on making the simulation ID generation more robust. Overall, these are good changes that improve the stability of the system.
| if time_step <= 0.0 || duration <= 0.0 { | ||
| return; | ||
| } | ||
| let mut simulations = self.simulations.lock().unwrap(); | ||
| if simulations.len() >= MAX_SIMULATIONS { | ||
| return; | ||
| } |
There was a problem hiding this comment.
While adding validation is a great improvement, silently returning on failure can make debugging difficult for the caller. It's not clear whether the simulation started or not, and if not, why.
Consider changing the function to return a Result to explicitly communicate success or failure. This is more idiomatic in Rust for functions that can fail.
You could define a custom error enum like this:
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum SimulationStartError {
InvalidParameters,
MaxSimulationsReached,
}Then, the function signature would become:
pub fn start_simulation(&self, time_step: f32, duration: f32) -> Result<(), SimulationStartError>
And the implementation would change to:
if time_step <= 0.0 || duration <= 0.0 {
return Err(SimulationStartError::InvalidParameters);
}
let mut simulations = self.simulations.lock().unwrap();
if simulations.len() >= MAX_SIMULATIONS {
return Err(SimulationStartError::MaxSimulationsReached);
}
// ... rest of the function
Ok(())This would provide clear feedback to the caller, allowing for proper error handling.
| return; | ||
| } | ||
| let new_simulation = Arc::new(Mutex::new(Simulation::new( | ||
| simulations.len() as u32 + 1, |
There was a problem hiding this comment.
The current method for generating a new simulation ID, simulations.len() as u32 + 1, is not robust. Since stop_simulation now clears the entire simulations vector, IDs will be reused when new simulations are started. This can cause confusion, for example when tracking simulations by ID in logs or external systems.
To ensure unique IDs, I recommend using an atomic counter within SimulationManager.
- Add an atomic counter to the
SimulationManagerstruct:
use std::sync::atomic::{AtomicU32, Ordering};
pub struct SimulationManager {
simulations: Arc<Mutex<Vec<Arc<Mutex<Simulation>>>>>,
next_id: AtomicU32,
}- Initialize it in
SimulationManager::new():
impl SimulationManager {
pub fn new() -> Self {
SimulationManager {
simulations: Arc::new(Mutex::new(Vec::new())),
next_id: AtomicU32::new(1),
}
}
// ...
}- Use it to generate the new ID:
let new_simulation = Arc::new(Mutex::new(Simulation::new(
self.next_id.fetch_add(1, Ordering::Relaxed),
time_step,
duration,
)));
There was a problem hiding this comment.
keep going with the cargo test output for shoutout
There was a problem hiding this comment.
concurred.
$ cargo test
Compiling simulation_engine v0.1.14 (/Users/niladri/Desktop/simulation-engine)
Finished `test` profile [unoptimized + debuginfo] target(s) in 2.76s
Running unittests engine/lib.rs (target/debug/deps/simulation_engine-16a5ad0d88731fa2)
running 14 tests
test physics::data_cleaner::tests::test_cleanup_inactive_objects ... ok
test physics::data_cleaner::tests::test_data_cleaner_configuration ... ok
test physics::data_cleaner::tests::test_data_cleaner_creation ... ok
test physics::data_cleaner::tests::test_cleanup_stats ... ok
test physics::physics_engine::tests::test_add_object ... ok
test physics::physics_engine::tests::test_collision_resolution ... ok
test physics::physics_engine::tests::test_gravity ... ok
test physics::data_cleaner::tests::test_needs_cleanup ... ok
test physics::physics_engine::tests::test_invalid_mass ... ok
test physics::physics_engine::tests::test_invalid_time_step ... ok
test physics::physics_engine::tests::test_set_gravity ... ok
test physics::physics_engine::tests::test_stress_large_timestep ... ok
test physics::physics_engine::tests::test_velocity_clamping ... ok
test physics::physics_engine::tests::test_wall_collision ... ok
test result: ok. 14 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Running unittests engine/main.rs (target/debug/deps/simulation_engine-6f20b0108946e79a)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Running tests/e2e_tests.rs (target/debug/deps/e2e_tests-fefafb32831242f2)
running 5 tests
test e2e_tests::test_e2e_pause_simulation ... ok
test e2e_tests::test_e2e_reset_simulation ... ok
test e2e_tests::test_e2e_start_simulation ... ok
test e2e_tests::test_e2e_get_simulations ... ok
test e2e_tests::test_e2e_stop_simulation ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.14s
Running tests/sdk_tests.rs (target/debug/deps/sdk_tests-107819e817854c5a)
running 5 tests
test test_physics_engine_add_object ... ok
test test_physics_engine_creation ... ok
test test_physics_engine_simulate ... ok
test test_simulation_manager_creation ... ok
test test_simulation_manager_start_simulation ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.21s
Running tests/simulation_tests.rs (target/debug/deps/simulation_tests-942e8a94248cf1a5)
running 5 tests
test tests::test_stop_simulation ... ok
test tests::test_pause_simulation ... ok
test tests::test_get_simulations ... ok
test tests::test_reset_simulation ... ok
test tests::test_start_simulation ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
Doc-tests simulation_engine
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
1 participant
Summary
This PR addresses multiple issues in the simulation engine:
start_simulationto prevent infinite loops when invalid time_step or duration values are provided.stop_simulationstops all active simulations to prevent resource exhaustion.start_simulationreturn aResultwith custom error types.Changes
engine/managers/simulation_manager.rs:time_step > 0andduration > 0instart_simulation.MAX_SIMULATIONSconstant set to 5.start_simulationto reject new simulations if the limit is reached.stop_simulationto stop all simulations and clear the list.SimulationStartErrorenum for explicit error communication.start_simulationto returnResult<(), SimulationStartError>.next_idcounter for unique simulation IDs.engine/main.rs:start_simulationresult.Issues Fixed
Testing
cargo checkandcargo clippy.