Clean up events more rigorously #163

UkoeHB · 2024-01-10T02:35:20Z

Moves ClientSet::ResetEvents before renet updates, to ensure events are cleared immediately before connecting.
ClientSet::ResetEvents runs every tick while waiting to connect to ensure the tick where you connect is as clean as possible.
Add system to discard client events while the client is waiting to connect. This is equivalent to letting client events hit the renet client, which will drop them.

codecov-commenter · 2024-01-10T02:53:02Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a6609a5) 83.37% compared to head (63d98bb) 90.35%.
Report is 4 commits behind head on master.

Files	Patch %	Lines
src/network_event/server_event.rs	60.00%	2 Missing ⚠️
src/network_event/client_event.rs	80.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
+ Coverage   83.37%   90.35%   +6.98%     
==========================================
  Files          20       19       -1     
  Lines        1287     1192      -95     
==========================================
+ Hits         1073     1077       +4     
+ Misses        214      115      -99

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Shatur · 2024-01-10T11:15:32Z

But why not just drain events once at disconnect?
Also running before the Renet's update will introduce one frame delay.
Here is how I would see it: #166.

UkoeHB · 2024-01-10T16:07:11Z

But why not just drain events once at disconnect?

Because it's best to mimic the renet interface as much as possible. Messages sent to renet while connecting will be dropped. It might seem like a good idea to be lenient and allow some message buffering, but in my experience that just makes reconnects ambiguous and way more difficult to handle properly (I ran into this with bevy_simplenet). It's best to make reconnects as clean and precise as possible, which means pessimistically dropping messages.

Also running before the Renet's update will introduce one frame delay.

Delay does not matter for dropping events. For precision, we drop everything until the moment renet connects.

Shatur · 2024-01-10T17:59:13Z

In Replicon we reset everything right after disconnect. I would prefer to keep it consistent. I don't think that #166 will affect robustness in any way.

UkoeHB · 2024-01-10T18:51:54Z

In Replicon we reset everything right after disconnect.

We only reset internal caches of the client that won't be used until the next replication message. Event resetting is an entirely different subject.

The only reason to not reset every tick is because you want to allow message buffering while connecting. In my opinion and experience that is not justified and makes the API harder to reason about.

Shatur · 2024-01-10T19:45:18Z

Resetting events every tick is just weird. Why doing so?
We either reset it before connect or after disconnect. And I prefer the later one since it's consistent with the rest of the codebase and what I would expect as a user.

UkoeHB · 2024-01-10T20:01:07Z

Resetting events every tick is just weird. Why doing so?

Resetting events is just a means to an end - we are discarded messages sent to a client that isn't connected. The semantics of a disconnected client need to be very precise: messages will fail to send in the span of time between a disconnect (which happens implicitly - we don't know precisely when messages start to fail) and a connect event. This ensures connect events cleanly mark the start of a new session.

Shatur · 2024-01-10T20:34:26Z

I understand your point, but I'm not sure if it's justifies running a system for each event to cleanup the event queue. It's not a cost-free check.
I think that a single cleanup after disconnect is fine, just advise users to avoid sending events before the connection.

UkoeHB · 2024-01-10T21:01:44Z

I think that a single cleanup after disconnect is fine, just advise users to avoid sending events before the connection.

This is a great example of my point. If we need to say "make an effort to do things right", then the API is suboptimal. The API should be right automatically.

I understand your point, but I'm not sure if it's justifies running a system for each event to cleanup the event queue. It's not a cost-free check.

We only run it when the client is not connected, which should definitely not be the hot path. If you are really concerned then we can .run_if(client_just_connected()) between RenetReceive and ClientSet::Receive.

Shatur · 2024-01-10T21:09:02Z

We only run it when the client is not connected

Right, I actually forgot about it. Then performance-wise it should be fine.
But I would prefer to run it before the connection, I think it will be a good middle-ground while still solves the mentioned problem.

src/network_event/server_event.rs

Shatur · 2024-01-10T21:30:58Z

src/network_event/server_event.rs

+    /// Gets the event queue length.
+    pub fn len(&mut self) -> usize {
+        self.0.values_len()
+    }
+
+    /// Tests whether there are any events queued.
+    pub fn is_empty(&mut self) -> bool {
+        self.len() == 0
+    }
+


Since we agree on #164, let's remove these methods and just use .0.

Shatur · 2024-01-10T21:31:57Z

src/client.rs

+                ClientSet::ResetEvents
+                    .after(NetcodeClientPlugin::update_system)
+                    .before(ClientSet::Receive)
+                    .run_if(not(has_authority()))


Do we need not(has_authority())?

One of the tests was breaking because local clients don't have a RenetClient (I guess?) so not(is_connected()) was always true and local resending wasn't working. The new run condition should not have that problem though, so I will remove this.

Shatur · 2024-01-10T21:33:27Z

src/client.rs

-    /// This is a separate set from `ClientSet::Reset` in case you want to enable/disable it separately.
-    /// In general it is best practice to clear queued server events after a disconnect, whereas you may want to
-    /// preserve replication state (in which case `ClientSet::Reset` should be disabled).


Maybe keep this part? Looks like a useful explanation.

src/network_event/client_event.rs

Shatur

Thanks!
Could you rebase to the latest master? Because there are some conflitcs because of my PR. And while you are at it, I left two more pure-style suggestions.

Will away from the computer for a couple hours, so I approving now, feel free to merge.

Shatur · 2024-01-10T22:45:40Z

src/client.rs

+use bevy_renet::{client_connected, client_just_connected, client_just_disconnected, renet::Bytes};
 use bevy_renet::{renet::RenetClient, transport::NetcodeClientPlugin, RenetClientPlugin};


While you are at it, could you group these imports, please?

Shatur · 2024-01-10T22:47:39Z

src/network_event/client_event.rs

@@ -139,6 +139,7 @@ impl ClientEventAppExt for App {
        self.add_event::<T>()
            .init_resource::<Events<FromClient<T>>>()
            .insert_resource(ClientEventChannel::<T>::new(channel_id))
+            .add_systems(PreUpdate, reset_system::<T>.in_set(ClientSet::ResetEvents))


Could you group all PreUpdate systems here and in server_event?
Just for consistency with Bevy and the rest of the codebase.

Shatur mentioned this pull request Jan 10, 2024

Cleanup events on disconnect #166

Closed

UkoeHB requested a review from Shatur January 10, 2024 21:32

Shatur requested changes Jan 10, 2024

View reviewed changes

Shatur approved these changes Jan 10, 2024

View reviewed changes

UkoeHB added 3 commits January 10, 2024 17:24

clean up events more rigorously

b8b3b5e

group imports

a54a03b

cleanup

7040211

UkoeHB force-pushed the event_cleanup branch from 63d98bb to 7040211 Compare January 10, 2024 23:27

UkoeHB enabled auto-merge (squash) January 10, 2024 23:28

UkoeHB merged commit 1871aff into projectharmonia:master Jan 10, 2024
5 checks passed

UkoeHB deleted the event_cleanup branch January 11, 2024 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up events more rigorously #163

Clean up events more rigorously #163

UkoeHB commented Jan 10, 2024

codecov-commenter commented Jan 10, 2024 •

edited

Loading

Shatur commented Jan 10, 2024 •

edited

Loading

UkoeHB commented Jan 10, 2024 •

edited

Loading

Shatur commented Jan 10, 2024

UkoeHB commented Jan 10, 2024 •

edited

Loading

Shatur commented Jan 10, 2024 •

edited

Loading

UkoeHB commented Jan 10, 2024 •

edited

Loading

Shatur commented Jan 10, 2024 •

edited

Loading

UkoeHB commented Jan 10, 2024

Shatur commented Jan 10, 2024

Shatur Jan 10, 2024

Shatur Jan 10, 2024

UkoeHB Jan 10, 2024 •

edited

Loading

Shatur Jan 10, 2024

Shatur left a comment

Shatur Jan 10, 2024

Shatur Jan 10, 2024

		use bevy_renet::{client_connected, client_just_connected, client_just_disconnected, renet::Bytes};
		use bevy_renet::{renet::RenetClient, transport::NetcodeClientPlugin, RenetClientPlugin};

Clean up events more rigorously #163

Clean up events more rigorously #163

Conversation

UkoeHB commented Jan 10, 2024

codecov-commenter commented Jan 10, 2024 • edited Loading

Codecov Report

Shatur commented Jan 10, 2024 • edited Loading

UkoeHB commented Jan 10, 2024 • edited Loading

Shatur commented Jan 10, 2024

UkoeHB commented Jan 10, 2024 • edited Loading

Shatur commented Jan 10, 2024 • edited Loading

UkoeHB commented Jan 10, 2024 • edited Loading

Shatur commented Jan 10, 2024 • edited Loading

UkoeHB commented Jan 10, 2024

Shatur commented Jan 10, 2024

Shatur Jan 10, 2024

Choose a reason for hiding this comment

Shatur Jan 10, 2024

Choose a reason for hiding this comment

UkoeHB Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Shatur Jan 10, 2024

Choose a reason for hiding this comment

Shatur left a comment

Choose a reason for hiding this comment

Shatur Jan 10, 2024

Choose a reason for hiding this comment

Shatur Jan 10, 2024

Choose a reason for hiding this comment

codecov-commenter commented Jan 10, 2024 •

edited

Loading

Shatur commented Jan 10, 2024 •

edited

Loading

UkoeHB commented Jan 10, 2024 •

edited

Loading

UkoeHB commented Jan 10, 2024 •

edited

Loading

Shatur commented Jan 10, 2024 •

edited

Loading

UkoeHB commented Jan 10, 2024 •

edited

Loading

Shatur commented Jan 10, 2024 •

edited

Loading

UkoeHB Jan 10, 2024 •

edited

Loading