switchupcb / disgo

Create a Discord Bot in Go using this Discord API Wrapper. The next generation of Discord API Consumption.
Apache License 2.0
89 stars 2 forks source link

add Sharding #64

Closed switchupcb closed 1 year ago

switchupcb commented 1 year ago

This pull request implements #26 with the following changes:

*Tests for the Gateway Rate Limits were added in the commit 'add Gateway tests`:

However, TestGatewayGlobalRateLimit has been removed in commit 'remove TestGatewayGlobalRateLimit test' for the following reason:

switchupcb commented 1 year ago

The test originally failed due to an issue with the actual test. Once that was fixed, the issue is a data race occurring when one Session is writing to the *Client struct (e.g., bot.ApplicationID = ready.Application.ID) while another Session is performing a read operation (e.g., LogCommand).

So I must investigate the best way to handle this data race or declare it benign: It is most likely solved through heavy usage of an RWMutex.

switchupcb commented 1 year ago

Here is a solution to the issue above.

First, a quick recap on data race semantics within the disgo.*Client.

So these structs and their respective operations have sufficient data race protection.

The main issue with bot.ApplicationID is that it is referenced over 600 times.

However, the field is only written to three times in the entire repository (including examples):

// from ./_examples/bot/main.go L#79 demonstrating that the field exists.
bot.ApplicationID = ""

// from ./wrapper/tests/integration/coverage_test.go L#57 setting the application ID for subsequent requests.
bot.ApplicationID = app.ID

// OFFENDING LINE: from ./wrapper/session.go L#327 setting the application ID for logging purposes.
bot.ApplicationID = app.ID

This situation leaves the following questions:

Questions

Is referencing the bot's application ID necessary?

It is required for certain requests. So the user must have a way to store the application ID in the client.

What can we do about this issue?

There are a few options available:

  1. Ignore it.
  2. Remove the offending write.
  3. Make the field atomic or use synchronization primitives (at the cost of logging performance and cognitive overhead for everyone involved).
  4. Disable the data race detector for this test under the condition that it is benign.
  5. Remove the test.

Is making this value an atomic operation worth it?

For two writes? No.

A user (developer) is expected to set the application ID before the application is running. Especially when the user is using an operation that — they know — requires an application ID.

The only operations that require an application ID are certain requests. In other words, APPLICATION ID IS NOT USED IN SESSION LOGIC (except for logging purposes): The main reason that it's included is in the rare case that the user decides to run two applications within the same codebase.

A user is only expected to run a single bot within a single time, even though running multiple bots is supported by disgo. So the real question: Does the application ID of the bot need to be set in the session code (when the ready event is received)?

So remove the offending write.