Change Arc<Mutex<ShardManager>> to Arc<ShardManager>

[tazz4843]

Has the side effect of (theoretically)
fixing shutdown by removing the major deadlock

Required converting all &mut self methods on ShardManager to &self

Closes #2507

[tazz4843]

Accidentally opened as ready for merge, converted to draft as I don't want a round 2 of last time we tried fixing shutdown. I'm going to fire up a local bot as a test and leave it running for a few days.

[mkrasnitski]

Looks good to me - let's hope this works. Your doctests are failing, though.

[tazz4843]

Turns out this breaks poise as well, I'll make a PR to that as a companion.

[GnomedDev]

Is this a good idea? Have you tested running shutdown concurrently to see if that breaks anything? It worries me if this was written with the assumption that it's &mut self that a new class of bugs will be introduced (that cannot be caught by rustc, as we are mutating the state of the program via IO to discord).

[mkrasnitski]

The only fields of ShardManager that end up being mutated are shard_shutdown as well as the integers shard_index, shard_init, and shard_total. The former is now behind a Mutex that only gets locked in one place, so no opportunity for deadlocks on the same thread.

The other three fields all get written to only in .set_shards. There could feasibly be a race condition where a context switch happens between two of the calls to .store, and then an invariant of the manager might get broken? I don't know. Maybe we should synchronize the three atomic writes to make sure they can't get interrupted.

[tazz4843]

Have you tested running shutdown concurrently to see if that breaks anything?

As far as I can tell, shutdown is a completely idempotent function, even with these new changes. The worst that could happen is an extra irrelevant "timed out" warning after a successful shutdown, which would have happened previously anyways. shutdown simply fires a message and then waits for a response, and if it doesn't get it, times out with an error.

The other three fields all get written to only in .set_shards. There could feasibly be a race condition where a context switch happens between two of the calls to .store, and then an invariant of the manager might get broken? I don't know. Maybe we should synchronize the three atomic writes to make sure they can't get interrupted.

.set_shards is only ever called in one place, Client::start_connection, which takes &mut self on the Client, preventing concurrent calls. Regardless, we could synchronize the calls, however I'm not sure how we could go about that.

[tazz4843]

As with the tokio-tungstenite upgrade, failing test seems to be unrelated: dtolnay/proc-macro2#398

[tazz4843]

Test bot has been online for three days straight now without any heartbeat failures (through network loss etc etc). I think this has finally solved the issue.