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

feat(kernel): wake tasks on service registration #268

Merged
merged 2 commits into from
Sep 3, 2023
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Sep 3, 2023

Depends on #267

Currently, the Registry::connect_* methods return an error immediately
if the requested service is not found in the registry. Most client types
implement a from_registry method which retry in a loop when the
requested service is not found in the registry. These methods will wait
for a fixed (short) amount of time and then try again until the registry
returns the service.

This approach is quite inefficient, as we have to run a bunch of retry
loops that keep trying to access a service that may not be there. This
may happen several times before the service actually is registered,
especially when registering a service requires connecting to another
service.

This branch improves the efficiency of waiting for a service to be
registered. Now, rather than retrying with a fixed-duration sleep, we
instead have the Registry own a WaitCell which is woken whenever a
new service is registered. This wakes all takes potentially waiting to
connect, allowing them to re-check whether the service they want is in
the registry. This idea was initially proposed by @jamesmunns in a
comment on PR #259

Connections are now established using either Registry::connect, which
retries whenever a new service is registered, or Registry::try_connect
which never retries.

Additionally, we now have the capacity to indicate that a service is not
found and that the registry is full, by closing the WaitCell. In
this case, retrying will never succeed, because the registry is full and
if the service isn't already there, it will never be added. In this
case, the retrying methods will also return an error, rather than never
completing, so we avoid a potential task leak.

In order to make this change, we need to move the RwLock from being
around the entire Registry to being inside the registry, around
items. This allows the WaitCell to be accessed regardless. It also
allows us to shorten the duration for which the lock is held. This
requires changing all methods on Registry to take &self.
Therefore, I've removed the wrapper methods on Kernel for connecting
and registering, since they can now just be called on kernel.registry
without a bunch of extra boilerplate for lock management. I've also
simplified the API surface of the registry a bit by removing the
connect methods that don't take a Hello, and just using
Registry::connect(()) in those cases. IMO, those methods weren't
really pulling their weight, and they required us to have a method named
Registry::try_connect_userspace_with_hello if we were going to add a
non-retrying connect variant. Now, we can just have
Registry::try_connect_userspace, Registry::connect_userspace,
Registry::connect, and Registry::try_connect, which feels much less
egregious.

@hawkw hawkw added kind: enhancement New feature or request area: kernel Related to the cross-platform kernel labels Sep 3, 2023
@hawkw hawkw self-assigned this Sep 3, 2023
Base automatically changed from eliza/shared-registry to main September 3, 2023 21:46
Depends on #267

Currently, the `Registry::connect_*` methods return an error immediately
if the requested service is not found in the registry. Most client types
implement a `from_registry` method which retry in a loop when the
requested service is not found in the registry. These methods will wait
for a fixed (short) amount of time and then try again until the registry
returns the service.

This approach is quite inefficient, as we have to run a bunch of retry
loops that keep trying to access a service that may not be there. This
may happen several times before the service actually is registered,
especially when registering a service requires connecting to another
service.

This branch improves the efficiency of waiting for a service to be
registered. Now, rather than retrying with a fixed-duration sleep, we
instead have the `Registry` own a `WaitCell` which is woken whenever a
new service is registered. This wakes all takes potentially waiting to
connect, allowing them to re-check whether the service they want is in
the registry. This idea was initially proposed by @jamesmunns in a
[comment] on PR #259

Connections are now established using either `Registry::connect`, which
retries whenever a new service is registered, or `Registry::try_connect`
which never retries.

Additionally, we now have the capacity to indicate that a service is not
found *and* that the registry is full, by closing the `WaitCell`. In
this case, retrying will never succeed, because the registry is full and
if the service isn't already there, it will never be added. In this
case, the retrying methods will also return an error, rather than never
completing, so we avoid a potential task leak.

In order to make this change, we need to move the `RwLock` from being
around the entire `Registry` to being inside the registry, around
`items`. This allows the `WaitCell` to be accessed regardless. It also
allows us to shorten the duration for which the lock is held. This
requires changing all methods on `Registry` to take `&self`.
Therefore, I've removed the wrapper methods on `Kernel` for connecting
and registering, since they can now just be called on `kernel.registry`
without a bunch of extra boilerplate for lock management. I've also
simplified the API surface of the registry a bit by removing the
`connect` methods that don't take a `Hello`, and just using
`Registry::connect(())` in those cases. IMO, those methods weren't
really pulling their weight, and they required us to have a method named
`Registry::try_connect_userspace_with_hello` if we were going to add a
non-retrying `connect` variant. Now, we can just have
`Registry::try_connect_userspace`, `Registry::connect_userspace`,
`Registry::connect`, and `Registry::try_connect`, which feels much less
egregious.

[comment]: #258 (comment)
Copy link
Contributor

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, dope as hell.

@hawkw hawkw enabled auto-merge (squash) September 3, 2023 22:00
@hawkw hawkw merged commit efbc428 into main Sep 3, 2023
7 checks passed
@hawkw hawkw deleted the eliza/connect2 branch September 3, 2023 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: kernel Related to the cross-platform kernel kind: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants