Improve components lifetime #1204

davfsa · 2022-07-06T01:08:33Z

Short summary

Bit of a rewrite of hikari internals to improve the lifetime, stability and consistency of the components.

Extended summary

Breaking changes

GatewayBot.join's until_close argument removed.
GatewayShardImpl.get_user_id is no longer async and will now always be available.

Bugfixes

GatewayShardImpl can now be instantiated out of an async environment for consistency with other components.
Correct signal handling in RESTBot.

Improvements

GatewayBot:
- General speedups.
- Fix a lot of edge cases of hard crashes if the application shuts unexpectedly.
- More consistent signal handling.
- run' shard_ids argument can now be a typing.Sequence.
- Improved logging.
RESTBot:
- Consistent signal handling inline with GatewayBot.
- Improved logging.
- Improved loop closing.
GatewayShardImpl:
- New is_connected property to determine whether the shard is connected to the gateway.
- Faster websocket pulling and heartbeating.
- Improved error handling.
- New gateway reconnect logic to account for resume_gateway_url.
- Rate limiting changes:
  - Chunking no longer has its own special ratelimit. Now it is shared with the rest of
    "non-priority" packages sent, which is of 117/60s (3 less than the hard limit).
    - "priority" packages currently only include hearbeating.

To @FasterSpeeding: I didnt realize until now how many "small fixes" there are in this pr, which makes it a bit hard to review. If you want, I could make smaller separate PRs for some of the issues if they are better for you to review

- Extract signal handling and use in both RESTBot and GatewayBot - RESTBot would previously rely on aiohttp, which would close the loop without handing it back to us before - More consistent, simplified and reliable lifetime behaviour across the different components - This obviously comes with a little speedup too :) - Simplify and add tests

davfsa · 2022-08-05T14:37:48Z

Implemented discord/discord-api-docs#5282 as part of 5c1b5e7.

If this PR were not to be merged before the 7th of August, I will open another smaller PR to merge directly into master (or if @FasterSpeeding rathers)

And bump for review :)

- This new bucket will account for all packages sent except "priority" ones, like heartbeating

davfsa · 2022-09-22T07:57:00Z

@FasterSpeeding Bump :)

changes/1204.feature.md

FasterSpeeding · 2022-09-24T13:24:45Z

hikari/traits.py

-        status: presences.Status = presences.Status.ONLINE,
-        shard_ids: typing.Optional[typing.AbstractSet[int]] = None,
-        shard_count: typing.Optional[int] = None,
-    ) -> None:


this is a pretty important breaking change which should be in the changelog, also why was this removed?

For consistency with RESTBotAware. They are both now Runnable and how their lifetime in regards to parameters to start, run, join and close are left up to implementation detail.

I forgot to add it to the changelog, will do so

For consistency with a class where all the run and start args were considered impl specific you removed the generic run and start args from the trait for a class where some of the run and start args were considered a part of the abstract interface since they were discord detail rather than bot impl detail?

They do not have to be Discord detail. They are left up to however the developers want to define them. The GatewayBotAware trait just specifies all the components it should have and thats about it. There is no reason to enforce this as abstraction detail here when it comes to how it should run

There is no reason to enforce this as abstraction detail here when it comes to how it should run.

There is? so ppl can run or start a hikari gateway bot (in-memory shard manager) generically while still being able to control what it declares to Discord on startup (which is what most of these args are for).

Also the field close_passed_executor make sense there for the run method since these are both ExecuterAware (meaning they manage an executor internally) and I'd argue missing that field for RESTBotAware.run was a mistake.

In the same way we dont enforce what the parameters to start, join and close a shard or a RESTBot should be, we shouldnt be enforcing it for GatewayBot either. Our API standard says that they should be runnable, but it stops there.

With the example you gave before of ExecutorAware, we never enforce how that executor should be handled, we just type that an ExecutorAware object will have a executor property.

In the same way we dont enforce what the parameters to start, join and close a shard or a RESTBot should be, we shouldnt be enforcing it for GatewayBot either. Our API standard says that they should be runnable, but it stops there.

There's a big distinction here though, all gateway bots will do stuff like declare an activity and spawn shards on startup. Rest bots don't have anything like that which is why it doesn't have any extra run or start arguments (but should have close_passed_executor on run, this was just missed originally).

With the example you gave before of ExecutorAware, we never enforce how that executor should be handled,

Wdym, the library's interface was enforcing it and now you're removing that.

In the same way we dont enforce what the parameters to start, join and close a shard

Even just having the start and close methods on Shard is enforcing behaviour, its the same thing as having optional arguments. The start and close methods make no sense for a Shard impl which just wraps around a shared message queue as an example and yet its still enforced. Start methods also aren't how you'd want to handle this for a gateway bot impl or shard impl which targets trio or anyio and having start methods on the interface like this is exposing asyncio specific async structuring.

The Shard start and close methods also don't really make sense for GatewayBot to expose considering it assumes that none of the shards within it are ever directly disabled in places, assuming it'll be the only thing managing their lifetimes.

We have as an API design for Shard to be be Runnable (this includes having a start, join and close). I am aware that you can have shards without those methods, but by our API design, they should. If someone doesn't want to use this API design, they can define and make their own.

But just like we enforce a shard being runnable, we don't enfoce it to have any specific arguments. This is done in Shard, RESTBotAware, and now GatewayBotAware.

I don't think we should enfoce those arguments at a trait level, which is why I removed them in this pr.

We have as an API design for Shard to be be Runnable (this includes having a start, join and close). I am aware that you can have shards without those methods, but by our API design, they should. If someone doesn't want to use this API design, they can define and make their own.

Ok so that wouldn't be compatible with Hikari at all so you're just saying "don't use Hikari" here and also why does it need to be a part of the interface? is there even any reason for shard to be directly runnable on the abstract interface when this it even breaks compatibility with the standard gateway bot impl let alone custom implementations.

But just like we enforce a shard being runnable, we don't enfoce it to have any specific arguments. This is done in Shard, RESTBotAware, and now GatewayBotAware.

Also that's just wrong, the only reason no args are enforced for shard.start is because it just straight out doesn't take any args on the standard implementation (since api shard.start is just a copy of the standard implementation's start method) and for RESTBotAware.run the reason is because I just missed the close executor arg when i implement it.

FasterSpeeding · 2022-09-25T15:49:36Z

hikari/impl/shard.py

+        self._is_closing = False
+        self._logger.info("shard shutdown successfully")
+
+    def get_user_id(self) -> snowflakes.Snowflake:


what's the point of this breaking change?

With the new lifetime, we can ensure it will always be available, so there is no need for it to be async anymore

If the shard has not connected fully yet, this should wait until the ID is set before returning.

The old code was just waiting until the shard had started to return the user (and returning if the shard was already active); how are you ensuring that it's available before the shard has started?

With the new lifetime, we can ensure that after start completes, we will have the information available and there wont be an attempt to access it until we return from start, which is when it would be reasonably expected for the information to be available. There is no need for the function to be async, as you shouldnt be waiting for the user_id if you never start the shard.

There is no need for the function to be async, as you shouldnt be waiting for the user_id if you never start the shard.

Ppl could call that method before the shard has started, this is why it was async before to wait for the shard to start. Idk what you're getting at here and its still impl detail as to when shards will be started vs populated so changing the standard interface for a impl specific thing like this feels wrong esp considering this is straight out outside the scope of shard like ppl can just start a shard by itself that is perfectly valid

davfsa added the enhancement New feature or request label Jul 6, 2022

davfsa force-pushed the task/new-lifetime branch 2 times, most recently from e699458 to 19f3a40 Compare July 6, 2022 21:20

davfsa added the optimization Optimizations to the code label Jul 6, 2022

davfsa force-pushed the task/new-lifetime branch 9 times, most recently from 3bb4554 to 76d29a5 Compare July 9, 2022 18:25

davfsa force-pushed the task/new-lifetime branch 2 times, most recently from 12c624f to fc4084a Compare July 17, 2022 11:14

davfsa requested a review from FasterSpeeding July 17, 2022 11:17

davfsa marked this pull request as ready for review July 17, 2022 11:17

davfsa force-pushed the task/new-lifetime branch from fc4084a to 75cd341 Compare July 17, 2022 11:30

davfsa enabled auto-merge (squash) July 17, 2022 11:30

davfsa force-pushed the task/new-lifetime branch from 75cd341 to 46f83b6 Compare August 1, 2022 11:13

davfsa added 2 commits August 3, 2022 13:49

Merge branch 'master' into task/new-lifetime

0a0f0be

Fix bad merge conflicts resolving

64fc41e

davfsa force-pushed the task/new-lifetime branch from 3944ad2 to 64fc41e Compare August 3, 2022 11:59

New gateway reconnecting logic

5c1b5e7

davfsa force-pushed the task/new-lifetime branch from 1e68a57 to 5c1b5e7 Compare August 5, 2022 14:36

Merge branch 'master' into task/new-lifetime

46e5471

davfsa requested review from FasterSpeeding and removed request for FasterSpeeding August 29, 2022 16:22

davfsa force-pushed the task/new-lifetime branch 2 times, most recently from b0f4ffb to 4f978e0 Compare August 30, 2022 17:49

Change chunking ratelimit to a non-priority rate limit

62f7094

- This new bucket will account for all packages sent except "priority" ones, like heartbeating

davfsa force-pushed the task/new-lifetime branch from 4f978e0 to 62f7094 Compare August 30, 2022 17:57

davfsa mentioned this pull request Sep 11, 2022

Threads #811

Merged

2 tasks

davfsa added 5 commits September 15, 2022 10:35

Account for query strings in gateway urls

4bee342

Handle more initial connection errors to attempt to recover from them

b4afd38

Merge branch 'master' into task/new-lifetime

40bb5aa

Fix failing tests

48bfb6f

Merge branch 'master' into task/new-lifetime

2beff27

Merge branch 'master' into task/new-lifetime

0beeb2b

davfsa force-pushed the task/new-lifetime branch from 8945c3f to 92c2c18 Compare September 24, 2022 08:11

Small cleanups

d7f81f6

davfsa force-pushed the task/new-lifetime branch from 92c2c18 to d7f81f6 Compare September 24, 2022 08:15

davfsa disabled auto-merge September 24, 2022 13:18

davfsa merged commit 8d520f1 into hikari-py:master Sep 24, 2022

FasterSpeeding reviewed Sep 24, 2022

View reviewed changes

FasterSpeeding reviewed Sep 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve components lifetime #1204

Improve components lifetime #1204

davfsa commented Jul 6, 2022 •

edited

Loading

davfsa commented Aug 5, 2022

davfsa commented Sep 22, 2022

FasterSpeeding Sep 24, 2022

davfsa Sep 24, 2022

FasterSpeeding Sep 24, 2022

davfsa Sep 24, 2022

FasterSpeeding Sep 25, 2022

davfsa Sep 25, 2022

FasterSpeeding Sep 25, 2022 •

edited

Loading

FasterSpeeding Sep 25, 2022 •

edited

Loading

davfsa Sep 26, 2022

FasterSpeeding Sep 26, 2022

FasterSpeeding Sep 25, 2022

davfsa Sep 25, 2022

FasterSpeeding Sep 25, 2022

davfsa Sep 25, 2022

FasterSpeeding Sep 25, 2022 •

edited

Loading

Improve components lifetime #1204

Improve components lifetime #1204

Conversation

davfsa commented Jul 6, 2022 • edited Loading

Short summary

Extended summary

Breaking changes

Bugfixes

Improvements

davfsa commented Aug 5, 2022

davfsa commented Sep 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FasterSpeeding Sep 25, 2022 • edited Loading

Choose a reason for hiding this comment

FasterSpeeding Sep 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FasterSpeeding Sep 25, 2022 • edited Loading

Choose a reason for hiding this comment

davfsa commented Jul 6, 2022 •

edited

Loading

FasterSpeeding Sep 25, 2022 •

edited

Loading

FasterSpeeding Sep 25, 2022 •

edited

Loading

FasterSpeeding Sep 25, 2022 •

edited

Loading