Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enable connection retrying on discord connector #24189

Merged
merged 2 commits into from
Jul 17, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Jul 12, 2023

We disabled retry logic in #7332 due to reported overheads (in stable), as a precautionary measure. I've revisited this (triggered by this report) as it's pretty bad not having any kind of retry logic.

Over 5 failed connection attempts:

Before After
Parallels Desktop 2021-09 at 09 36 44 Parallels Desktop 2023-07-12 at 09 34 33

Note that this is running time, not all time. It does not include actual Thread.Sleep sleep time, and the overhead seen here is actual CPU time being used.

Of note, the implementation of NamedPipeClientStream has some egregious use of SpinOnce:

https://github.com/dotnet/runtime/blob/d59af2cf097acb100ad6a9cba652be34be6f4a2e/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeClientStream.cs#L151-L155

This adds perceivable overhead to the connect process, which we really don't want on a background thread.

To get around this, I've made a local copy of ManagedNamedPipeClient (https://github.com/Lachee/discord-rpc-csharp/blob/master/DiscordRPC/IO/ManagedNamedPipeClient.cs) and adjusted the connect timeout to 0, removing this overhead completely.

Tested on macOS and windows.

  • Test on linux (initial connection and reconnect after closing and reopening discord, should occur within 10 seconds)

When testing, this diff can be used to enable console log output:

diff --git a/osu.Desktop/DiscordRichPresence.cs b/osu.Desktop/DiscordRichPresence.cs
index 1ea85ee2e2..3446f39743 100644
--- a/osu.Desktop/DiscordRichPresence.cs
+++ b/osu.Desktop/DiscordRichPresence.cs
@@ -4,6 +4,7 @@
 using System;
 using System.Text;
 using DiscordRPC;
+using DiscordRPC.Logging;
 using DiscordRPC.Message;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
@@ -16,6 +17,7 @@
 using osu.Game.Online.API.Requests.Responses;
 using osu.Game.Rulesets;
 using osu.Game.Users;
+using LogLevel = osu.Framework.Logging.LogLevel;
 
 namespace osu.Desktop
 {
@@ -51,6 +53,8 @@ private void load(OsuConfigManager config)
                 SkipIdenticalPresence = false // handles better on discord IPC loss, see updateStatus call in onReady.
             };
 
+            client.Logger = new ConsoleLogger(DiscordRPC.Logging.LogLevel.Trace);
+
             client.OnReady += onReady;
 
             client.OnError += (_, e) => Logger.Log($"An error occurred with Discord RPC Client: {e.Code} {e.Message}", LoggingTarget.Network);

@bdach
Copy link
Collaborator

bdach commented Jul 12, 2023

Test on linux (initial connection and reconnect after closing and reopening discord, should occur within 10 seconds)

I've tested this, and it seems to work, but sorry, I can't just not ask...

To get around this, I've made a local copy of ManagedNamedPipeClient (https://github.com/Lachee/discord-rpc-csharp/blob/master/DiscordRPC/IO/ManagedNamedPipeClient.cs) and adjusted the connect timeout to 0, removing this overhead completely.

We're adding a local copy of a 500-line class? For a one-liner change? Which I see basically no reason for upstream to reject (and we haven't even asked if they would anyways)? And so now we're on the hook for maintaining this code and keeping it up to date with upstream too, with the one-liner on top? It's a little baffling if you ask me. Maybe upstream will take a while to merge that but it's not like this is a life-or-death change?

Like sure, we've done similar things with humanizer, but that's because humanizer upstream is practically dead (no new releases in 1.5 years, very few actual changes on master, issue triage is basically nonexistent). discord-rpc-csharp is booming with activity by comparison, and they probably don't even need to change their code that often anyways.

I know we've been increasingly on the "roll your own" train as time goes on but this feels like pushing it.

@peppy
Copy link
Member Author

peppy commented Jul 13, 2023

I'll PR the change upstream but have little hope as the library has not been updated in years.

@peppy
Copy link
Member Author

peppy commented Jul 13, 2023

Upstream PR submitted: Lachee/discord-rpc-csharp#237

@peppy
Copy link
Member Author

peppy commented Jul 16, 2023

I've force pushed this with a package update rather than local implementation (see Lachee/discord-rpc-csharp#237).

@bdach bdach changed the title Implement a more efficient discord connector and re-enable connection retrying Re-enable connection retrying on discord connector Jul 17, 2023
@bdach
Copy link
Collaborator

bdach commented Jul 17, 2023

The package update is nice and all, but you also force-pushed the part that was disabling the retry mechanism out, too. I brought it back in 17aac06.

Probably worth a go as to how this behaves in the wild...

@bdach bdach merged commit 551c243 into ppy:master Jul 17, 2023
@peppy peppy deleted the discord-rpc-reconnect-fix branch July 18, 2023 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants