Common Gateway Implementation

[carlst99]

This PR provides my take on a common gateway infrastructure. As to whether or not it is sensible, I'll leave to you kind reviewers 😄.

Here is a brief overview of the changes.

The IGatewayClient

This defines the interface for a gateway client. This is similar to the public interface of the current DiscordGatewayClient, with the addition of the 'pre-shutdown commands' concept. This allows consumers to de/register commands that they wish to be sent immediately before a user-requested shutdown is initiated. The primary use case for this is the voice gateway needing to update its voice status when the program is terminated.

Currently, the IGatewayClient is not registered to the service collection.

The BaseGatewayClient

This abstract class handles the sending and receiving of gateway messages, and maintains the gateway connection and dispatch service. It expects the implementor to provide logic for connection and heartbeat sending, and provides an opportunity for implementors to process payloads as they are received.

The DiscordGatewayClient

This has been refactored to implement the BaseGatewayClient. This has largely been a copy-paste operation; the main changes can be found in the heart-beating logic and the offhanding of send/receive to the base.

IResponderDispatchService and DefaultResponderDispatchService

These are rather similar to the ResponderDispatchService that was recently introduced. They represent a means to dispatch payloads to relevant responders, and to handle the results of any responder execution.

To compare to the current implementation, my dispatch service uses a blocking run method, rather than start and stop. Hence, the consumer must manage the run task. I believe this has the advantage of the consumer knowing if a fatal error has occurred within the dispatch service, and can restart it correspondingly.

Further, this implementation expects IGatewayEvent instances to be enqueued, rather than IPayload instances.

Finally, I use an unbounded channel to enqueue payloads. My logic here being that I don't want to block the gateway receiver. Would very much like to hear more thoughts on this though; I can see that being unable to dispatch events fast enough is as good as having a blocked receiver.

The WebSocketPayloadTransportService

I have made significant changes to this service. Primarily, I was aiming to reduce memory consumption. Relevant benchmarks are attached below.

I've also adjusted how new ClientWebSocket instances are resolved, and how the socket receive task is cancelled. These changes prevent the socket from being prematurely disposed or aborted upon any cancellation, allowing a proper close handshake to be completed.

Payload Deserialization Benchmarks.

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
AMD Ryzen 5 3600, 1 CPU, 12 logical and 6 physical cores
.NET SDK=6.0.201
  [Host]     : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT
  DefaultJob : .NET 6.0.3 (6.0.322.12309), X64 RyuJIT


|                                       Method |     Mean |    Error |   StdDev |   Median | Ratio | RatioSD |  Gen 0 |  Gen 1 | Allocated |
|--------------------------------------------- |---------:|---------:|---------:|---------:|------:|--------:|-------:|-------:|----------:|
|                           CurrentDeserialize | 91.39 us | 1.817 us | 4.592 us | 89.84 us |  1.00 |    0.00 | 7.3242 | 1.4648 |     60 KB |
| CurrentDeserializeWithRecyclableMemoryStream | 77.76 us | 0.596 us | 0.557 us | 77.58 us |  0.81 |    0.04 | 5.8594 | 1.0986 |     48 KB |

Payload Serialization Benchmarks.

|              Method |     Mean |    Error |   StdDev | Ratio | RatioSD |  Gen 0 | Allocated |
|-------------------- |---------:|---------:|---------:|------:|--------:|-------:|----------:|
|    CurrentSerialize | 11.91 us | 0.228 us | 0.244 us |  1.00 |    0.00 | 0.4272 |   3,592 B |
|        NewSerialize | 10.76 us | 0.154 us | 0.137 us |  0.91 |    0.02 | 0.0458 |     424 B |

Next Steps

More thorough unit testing might be needed; for example to ensure that pre-shutdown commands are properly sent. I will also be testing this revised gateway in my bots over a number of days, to see if anything trips it up.

[carlst99]

Testing in my bot over a longer period has shown there is a significant issue somewhere; the bot keeps failing to make interaction responses in time. This likely means that either the receive loop or dispatch logic has a bottleneck somewhere. Regardless, this isn't ready to see the light of day.

[Nihlus]

Awesome work! I definitely like the blocking Run method better than the Start/Stop pattern I currently use.

Unfortunately, I'm swamped with a lot of other stuff at the moment, so I won't be able to do a proper review for a while - that said, it sounds like you might need some time to get things into a stable state.

Drop a comment here when you're reading for a thorough review, but overall, I like the design.

[carlst99]

Thanks! No problem at all, I'll leave an update when I think it's ready to go. I certainly need some time to look into a couple of issues, and that may take a little while myself.

[Foxtrek64]

I wonder if some of this should be pulled and sent up to Remora.Rest or Remora.Rest.Gateway. I could see the this being useful for more than just Discord.

[AraHaan]

Wait why do I see await calls not call .ConfigureAwait(false); here (in this PR) to avoid the chance have deadlocking every thread in the any program that use remora?

[Nihlus]

...because that's not what ConfigureAwait(false) is for?

[carlst99]

Testing in my bot over a longer period has shown there is a significant issue somewhere; the bot keeps failing to make interaction responses in time...

Further testing has proven the implementation to appear far more stable now. I'm less sure that it's to do with any changes I've made, rather than that my test bed had a couple of glaring flaws 😬. I've also run the SlashCommands sample through the dotTrace profiler, and can't see anything that looks out of the ordinary. I'm not very familiar with profiling, mind.

I'd still like to do another pass over the code before a full-on review, but I think it's closer to a ready state now.

[VelvetToroyashi]

It seems you dropped support for OCE when logging errors?

[carlst99]

Yes, either an OperationCanceledException or TaskCanceledException should indicate a user-requested shutdown. I didn't feel a need to log them, but it's no harm to do so.

[VelvetToroyashi]

Ah. Sorry, missed line 109 somehow

[VelvetToroyashi]

Cant we have this be instead of 50 to be lower to like 30 or 20?

Why would we want to burn more CPU cycles? 50ms is fine imho.

[AraHaan]

Can this be worked on more? I would most certainly like this.

[carlst99]

Can this be worked on more? I would most certainly like this.

Sorry for the slow reply. I've had a number of other projects on my plate. With the existing gateway having moved ahead and not foreseeing any immediate time to restart work on the voice implementation, I (likely mistakenly) let this fall by the wayside.

I'll likely start over and rebase relevant changes onto the current gateway implementation. I'll scope this out over the next week and update then if I think I'll have it done in any sort of reasonable timeframe. Otherwise, probably best to drop it so other people feel more free to PR their own implementation.

[AraHaan]

That is ok, as long as we can eventually get a common gateway that would be ok with me.

I would also be ok with making the common gateway, but split it into it's own library that is not specific to discord as well.

[Foxtrek64]

Having a generic gateway client in its own library (maybe Remora.Rest.Gateway?) could be rather useful. I can personally think of a couple places where I'd have a use for it, so I'm sure I wouldn't be alone here.

[Nihlus]

I don't think we should prioritize breaking the gateway out into its own library right now; not sure if there's a need for it.

[AraHaan]

I could see a need for it for cases like:

• you want to make a custom wrapper to an api with a gateway similar to Discord's.

For that case a remora generic gateway library would be a great idea and also allows more code to be shared as well (that is not only Discord specific gateway stuff).

This is why I think the common gateway should be placed in a separate non-discord specific package that the this discord repository could depend on.

[carlst99]

I don't think this is suitable to break out into a generic library. Most of the components have logic that's tailored towards Discord's error codes and connection specifics. The event dispatch system is free of this, but I'd argue it's fairly easy to replicate, and I imagine many people will be using their own dispatch mechanisms regardless.

Anecdotally, I maintain a library which also wraps an API with a websocket event stream. I'd not be able to reuse the gateway client, and would be making a number of changes to the transport service. My dispatch system is similar, but has a few critical differences. To write Remora's gateway logic in such a way that it's generic would require an impractically high level of abstraction, I feel.

[carlst99]

Well, we're another two months down the line and I've still made no progress on this 😓. I still hope to revisit this at some stage, but in the meantime I'm going to close this PR, particularly given how out of date it currently is.