From 855ae7300745123c24ca116a6ade6da76af6f625 Mon Sep 17 00:00:00 2001 From: koe Date: Fri, 12 Jan 2024 14:53:01 -0600 Subject: [PATCH 1/6] don't panic on race condition in client info updating --- src/server.rs | 15 ++++----- src/server/clients_info.rs | 44 ++++++++++++++++++------- tests/replication.rs | 67 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 21 deletions(-) diff --git a/src/server.rs b/src/server.rs index 0467d977..1640df94 100644 --- a/src/server.rs +++ b/src/server.rs @@ -1,4 +1,4 @@ -pub(super) mod clients_info; +pub mod clients_info; pub(super) mod despawn_buffer; pub(super) mod removal_buffer; pub(super) mod replicated_archetypes_info; @@ -341,9 +341,8 @@ fn collect_changes( component, )?; } else { - let tick = *client_info - .ticks - .get(&entity.entity()) + let tick = client_info + .get_change_limit(entity.entity()) .expect("entity should be present after adding component"); if ticks.is_changed(tick, change_tick.this_run()) { update_message.write_component( @@ -363,9 +362,7 @@ fn collect_changes( // If there is any insertion or we must initialize, include all updates into init message // and bump the last acknowledged tick to keep entity updates atomic. init_message.take_entity_data(update_message)?; - client_info - .ticks - .insert(entity.entity(), change_tick.this_run()); + client_info.set_change_limit(entity.entity(), change_tick.this_run()); } else { update_message.end_entity_data()?; } @@ -425,7 +422,7 @@ fn collect_despawns( for entity in despawn_buffer.drain(..) { let mut shared_bytes = None; for (message, _, client_info) in messages.iter_mut_with_info() { - client_info.ticks.remove(&entity); + client_info.remove_despawned(entity); message.write_entity(&mut shared_bytes, entity)?; } } @@ -451,7 +448,7 @@ fn collect_removals( for (message, _, client_info) in messages.iter_mut_with_info() { message.start_entity_data(entity); for &replication_id in components { - client_info.ticks.insert(entity, tick); + client_info.set_change_limit(entity, tick); message.write_replication_id(replication_id)?; } message.end_entity_data(false)?; diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index cb888c4e..ede7a036 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -7,10 +7,6 @@ use bevy::{ }; use bevy_renet::renet::ClientId; -/// Stores meta-information about connected clients. -#[derive(Default, Resource)] -pub(crate) struct ClientsInfo(Vec); - /// Reusable buffers for [`ClientsInfo`] and [`ClientInfo`]. #[derive(Default, Resource)] pub(crate) struct ClientBuffers { @@ -25,9 +21,13 @@ pub(crate) struct ClientBuffers { entities: Vec>, } +/// Stores meta-information about connected clients. +#[derive(Default, Resource)] +pub struct ClientsInfo(Vec); + impl ClientsInfo { /// Returns a mutable iterator over clients information. - pub(super) fn iter_mut(&mut self) -> impl Iterator { + pub fn iter_mut(&mut self) -> impl Iterator { self.0.iter_mut() } @@ -75,10 +75,10 @@ impl ClientsInfo { } } -pub(super) struct ClientInfo { +pub struct ClientInfo { id: ClientId, pub(super) just_connected: bool, - pub(super) ticks: EntityHashMap, + ticks: EntityHashMap, updates: HashMap, next_update_index: u16, } @@ -148,9 +148,22 @@ impl ClientInfo { (update_index, &mut update_info.entities) } + /// Sets the change limit for an entity that is replicated to this client. + /// + /// The change limit is the reference point for determining if components on an entity have changed and + /// need to be replicated. Component changes older than the change limit are assumed to be acked by the client. + pub(super) fn set_change_limit(&mut self, entity: Entity, tick: Tick) { + self.ticks.insert(entity, tick); + } + + /// Gets the change limit for an entity that is replicated to this client. + pub(super) fn get_change_limit(&mut self, entity: Entity) -> Option { + self.ticks.get(&entity).copied() + } + /// Marks update with the specified index as acknowledged. /// - /// Ticks for all entities from this update will be set to the update's tick if it's higher. + /// Change limits for all entities from this update will be set to the update's tick if it's higher. /// /// Keeps allocated memory in the buffers for reuse. pub(super) fn acknowledge( @@ -168,10 +181,10 @@ impl ClientInfo { }; for entity in &update_info.entities { - let last_tick = self - .ticks - .get_mut(entity) - .expect("tick should be inserted on any component insertion"); + let Some(last_tick) = self.ticks.get_mut(entity) else { + // We ignore missing entities, since they were probably despawned. + continue; + }; // Received tick could be outdated because we bump it // if we detect any insertion on the entity in `collect_changes`. @@ -188,6 +201,13 @@ impl ClientInfo { ); } + /// Removes a despawned entity tracked by this client. + pub fn remove_despawned(&mut self, entity: Entity) { + self.ticks.remove(&entity); + // We don't clean up `self.updates` for efficiency reasons. Self::acknowledge() will properly + // ignore despawned entities. + } + /// Removes all updates older then `min_timestamp`. /// /// Keeps allocated memory in the buffers for reuse. diff --git a/tests/replication.rs b/tests/replication.rs index 95f2a969..2ede0e4f 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -2,6 +2,7 @@ mod connect; use bevy::{prelude::*, utils::Duration}; use bevy_replicon::{prelude::*, scene}; +use bevy_replicon::server::clients_info::ClientsInfo; use bevy_renet::renet::{ transport::{NetcodeClientTransport, NetcodeServerTransport}, @@ -435,6 +436,72 @@ fn update_replication() { assert!(component.0); } +#[test] +fn update_and_removal_with_stale_ack_replication() { + let mut server_app = App::new(); + let mut client_app = App::new(); + for app in [&mut server_app, &mut client_app] { + app.add_plugins(( + MinimalPlugins, + ReplicationPlugins.set(ServerPlugin { + tick_policy: TickPolicy::EveryFrame, + ..Default::default() + }), + )) + .replicate::(); + } + + // 1. Connect a client. + connect::single_client(&mut server_app, &mut client_app); + + // 2. Spawn an entity and replicate it to the client. + let server_entity = server_app + .world + .spawn((Replication, BoolComponent(false))) + .id(); + + server_app.update(); + client_app.update(); + + assert_eq!(client_app.world.entities().len(), 1); + + // 3. Update the component and replicate it to the client. + // - The client will send an update ack. + let mut component = server_app + .world + .get_mut::(server_entity) + .unwrap(); + component.0 = true; + + server_app.update(); + client_app.update(); + + let component = client_app + .world + .query::<&BoolComponent>() + .single(&client_app.world); + assert!(component.0); + + // 4. Despawn the entity. + server_app + .world + .entity_mut(server_entity) + .despawn(); + + // 5. Detect the despawn before collecting the client ack. + // - Due to networking race conditions we need to do this manually. + server_app.world.resource_mut::().iter_mut().next().unwrap().remove_despawned(server_entity); + + // 6. Collect entity acks from the client and send the desapwn to the client. + // - Entity acks should be ignored for the despawned entity. + server_app.update(); + + // 7. Update the client + client_app.update(); + + assert_eq!(client_app.world.entities().len(), 0); +} + #[test] fn big_entity_update_replication() { let mut server_app = App::new(); From 3c03d22a3e963dcf8acfdfd48553108467821ee7 Mon Sep 17 00:00:00 2001 From: koe Date: Fri, 12 Jan 2024 14:54:39 -0600 Subject: [PATCH 2/6] fmt --- tests/replication.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/replication.rs b/tests/replication.rs index 2ede0e4f..0a4191d1 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -1,8 +1,8 @@ mod connect; use bevy::{prelude::*, utils::Duration}; -use bevy_replicon::{prelude::*, scene}; use bevy_replicon::server::clients_info::ClientsInfo; +use bevy_replicon::{prelude::*, scene}; use bevy_renet::renet::{ transport::{NetcodeClientTransport, NetcodeServerTransport}, @@ -483,14 +483,17 @@ fn update_and_removal_with_stale_ack_replication() { assert!(component.0); // 4. Despawn the entity. - server_app - .world - .entity_mut(server_entity) - .despawn(); + server_app.world.entity_mut(server_entity).despawn(); // 5. Detect the despawn before collecting the client ack. // - Due to networking race conditions we need to do this manually. - server_app.world.resource_mut::().iter_mut().next().unwrap().remove_despawned(server_entity); + server_app + .world + .resource_mut::() + .iter_mut() + .next() + .unwrap() + .remove_despawned(server_entity); // 6. Collect entity acks from the client and send the desapwn to the client. // - Entity acks should be ignored for the despawned entity. From 6b0953d50fd750a21067b4e6fb67637f62e6736c Mon Sep 17 00:00:00 2001 From: koe Date: Fri, 12 Jan 2024 15:09:15 -0600 Subject: [PATCH 3/6] changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cea042a..6c153aa7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Make `NetworkChannels` channel-creation methods public (`create_client_channel()` and `create_server_channel()`). - Implement `Eq` and `PartialEq` on `EventType`. +### Fixed + +- Don't panic when handling client acks if the ack references a despawned entity. + ## [0.19.0] - 2024-01-07 ### Added From 4afbe1df2892a9aa19c71bf04bcef86c36dd06c7 Mon Sep 17 00:00:00 2001 From: koe Date: Fri, 12 Jan 2024 16:10:34 -0600 Subject: [PATCH 4/6] review comments --- src/server/clients_info.rs | 8 ++++---- tests/replication.rs | 19 ++++++------------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index ede7a036..55fe9940 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -7,6 +7,10 @@ use bevy::{ }; use bevy_renet::renet::ClientId; +/// Stores meta-information about connected clients. +#[derive(Default, Resource)] +pub struct ClientsInfo(Vec); + /// Reusable buffers for [`ClientsInfo`] and [`ClientInfo`]. #[derive(Default, Resource)] pub(crate) struct ClientBuffers { @@ -21,10 +25,6 @@ pub(crate) struct ClientBuffers { entities: Vec>, } -/// Stores meta-information about connected clients. -#[derive(Default, Resource)] -pub struct ClientsInfo(Vec); - impl ClientsInfo { /// Returns a mutable iterator over clients information. pub fn iter_mut(&mut self) -> impl Iterator { diff --git a/tests/replication.rs b/tests/replication.rs index 0a4191d1..de496a53 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -1,8 +1,7 @@ mod connect; use bevy::{prelude::*, utils::Duration}; -use bevy_replicon::server::clients_info::ClientsInfo; -use bevy_replicon::{prelude::*, scene}; +use bevy_replicon::{prelude::*, scene, server::clients_info::ClientsInfo}; use bevy_renet::renet::{ transport::{NetcodeClientTransport, NetcodeServerTransport}, @@ -451,10 +450,8 @@ fn update_and_removal_with_stale_ack_replication() { .replicate::(); } - // 1. Connect a client. connect::single_client(&mut server_app, &mut client_app); - // 2. Spawn an entity and replicate it to the client. let server_entity = server_app .world .spawn((Replication, BoolComponent(false))) @@ -465,8 +462,7 @@ fn update_and_removal_with_stale_ack_replication() { assert_eq!(client_app.world.entities().len(), 1); - // 3. Update the component and replicate it to the client. - // - The client will send an update ack. + // The client will send an update ack. let mut component = server_app .world .get_mut::(server_entity) @@ -482,11 +478,10 @@ fn update_and_removal_with_stale_ack_replication() { .single(&client_app.world); assert!(component.0); - // 4. Despawn the entity. server_app.world.entity_mut(server_entity).despawn(); - // 5. Detect the despawn before collecting the client ack. - // - Due to networking race conditions we need to do this manually. + // Detect the despawn before collecting the client ack. + // Due to networking race conditions we need to do this manually. server_app .world .resource_mut::() @@ -495,13 +490,11 @@ fn update_and_removal_with_stale_ack_replication() { .unwrap() .remove_despawned(server_entity); - // 6. Collect entity acks from the client and send the desapwn to the client. - // - Entity acks should be ignored for the despawned entity. + // Collect entity acks from the client and send the desapwn to the client. + // Entity acks should be ignored for the despawned entity. server_app.update(); - // 7. Update the client client_app.update(); - assert_eq!(client_app.world.entities().len(), 0); } From 28937160c16c44e47fb4e9e239ad028e0b360fb5 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sat, 13 Jan 2024 01:40:08 +0200 Subject: [PATCH 5/6] Simplify test --- tests/replication.rs | 66 ++------------------------------------------ 1 file changed, 2 insertions(+), 64 deletions(-) diff --git a/tests/replication.rs b/tests/replication.rs index de496a53..bab93f45 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -1,7 +1,7 @@ mod connect; use bevy::{prelude::*, utils::Duration}; -use bevy_replicon::{prelude::*, scene, server::clients_info::ClientsInfo}; +use bevy_replicon::{prelude::*, scene}; use bevy_renet::renet::{ transport::{NetcodeClientTransport, NetcodeServerTransport}, @@ -435,69 +435,6 @@ fn update_replication() { assert!(component.0); } -#[test] -fn update_and_removal_with_stale_ack_replication() { - let mut server_app = App::new(); - let mut client_app = App::new(); - for app in [&mut server_app, &mut client_app] { - app.add_plugins(( - MinimalPlugins, - ReplicationPlugins.set(ServerPlugin { - tick_policy: TickPolicy::EveryFrame, - ..Default::default() - }), - )) - .replicate::(); - } - - connect::single_client(&mut server_app, &mut client_app); - - let server_entity = server_app - .world - .spawn((Replication, BoolComponent(false))) - .id(); - - server_app.update(); - client_app.update(); - - assert_eq!(client_app.world.entities().len(), 1); - - // The client will send an update ack. - let mut component = server_app - .world - .get_mut::(server_entity) - .unwrap(); - component.0 = true; - - server_app.update(); - client_app.update(); - - let component = client_app - .world - .query::<&BoolComponent>() - .single(&client_app.world); - assert!(component.0); - - server_app.world.entity_mut(server_entity).despawn(); - - // Detect the despawn before collecting the client ack. - // Due to networking race conditions we need to do this manually. - server_app - .world - .resource_mut::() - .iter_mut() - .next() - .unwrap() - .remove_despawned(server_entity); - - // Collect entity acks from the client and send the desapwn to the client. - // Entity acks should be ignored for the despawned entity. - server_app.update(); - - client_app.update(); - assert_eq!(client_app.world.entities().len(), 0); -} - #[test] fn big_entity_update_replication() { let mut server_app = App::new(); @@ -668,6 +605,7 @@ fn despawn_update_replication() { server_app.update(); client_app.update(); + server_app.update(); // Let server receive an update to trigger acknowledgment. assert!(client_app.world.entities().is_empty()); } From cb5cdc04c59cc69398a50f53d7d419513c5dcd64 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sat, 13 Jan 2024 01:40:18 +0200 Subject: [PATCH 6/6] Unpablic unused stuff I will make it pub in a separate PR. --- src/server/clients_info.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index 55fe9940..36471198 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -9,7 +9,7 @@ use bevy_renet::renet::ClientId; /// Stores meta-information about connected clients. #[derive(Default, Resource)] -pub struct ClientsInfo(Vec); +pub(crate) struct ClientsInfo(Vec); /// Reusable buffers for [`ClientsInfo`] and [`ClientInfo`]. #[derive(Default, Resource)] @@ -27,7 +27,7 @@ pub(crate) struct ClientBuffers { impl ClientsInfo { /// Returns a mutable iterator over clients information. - pub fn iter_mut(&mut self) -> impl Iterator { + pub(super) fn iter_mut(&mut self) -> impl Iterator { self.0.iter_mut() } @@ -75,7 +75,7 @@ impl ClientsInfo { } } -pub struct ClientInfo { +pub(super) struct ClientInfo { id: ClientId, pub(super) just_connected: bool, ticks: EntityHashMap, @@ -204,8 +204,8 @@ impl ClientInfo { /// Removes a despawned entity tracked by this client. pub fn remove_despawned(&mut self, entity: Entity) { self.ticks.remove(&entity); - // We don't clean up `self.updates` for efficiency reasons. Self::acknowledge() will properly - // ignore despawned entities. + // We don't clean up `self.updates` for efficiency reasons. + // `Self::acknowledge()` will properly ignore despawned entities. } /// Removes all updates older then `min_timestamp`.