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(service): Add notion of State #4188

Closed
wants to merge 6 commits into from

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented Oct 29, 2024

This PR introduces the idea of PublicState to our services, adding the notion of state to them. Such a state is constructed before the spawn_handler is called and returned with the Addr of the Service. This abstraction aims to unify how services expose state (both readable and writable, depending on the implementation).

In addition to the state, this PR adds:

  • shutdown parameter in the spawn_handler to make it more ergonomic to subscribe to shutdown signals in a service loop.
  • channel() method to create the tx and rx channels for service communication directly on the service itself.
  • start_with_receiver() method to handle the case in which a service has to be started with an injected rx channel due to circular dependencies between services.

This PR is just the first small step towards improving our Service framework.

Closes: #4180

@iambriccardo iambriccardo changed the title feat(service): Add notion of state feat(service): Add notion of State Oct 30, 2024
@iambriccardo iambriccardo marked this pull request as ready for review October 30, 2024 13:02
@iambriccardo iambriccardo requested a review from a team as a code owner October 30, 2024 13:02
Copy link
Contributor

@loewenheim loewenheim left a comment

Choose a reason for hiding this comment

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

It's unfortunately quite boilerplate-heavy. Would be nicer with associated type defaults but they're unstable :/

But on the whole this makes a lot of sense to me. Some services already effectively use this pattern, this just codifies it.

Comment on lines +902 to +903
/// A state interface for [services](`Service`).
pub trait State {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this trait intended to be expanded later? Currently I don't think this accomplishes anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, it was meant to be used as some sort of code documentation which clarifies when a type can be used as a state but it serves no purpose besides that. I don't know if it's common in Rust to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case I would just document the associated type directly.

relay-server/src/services/buffer/mod.rs Outdated Show resolved Hide resolved
@iambriccardo
Copy link
Member Author

It's unfortunately quite boilerplate-heavy. Would be nicer with associated type defaults but they're unstable :/

But on the whole this makes a lot of sense to me. Some services already effectively use this pattern, this just codifies it.

Yes, unfortunately without that feature it's a bit boilerplate heavy. I considered requiring the Default trait bound to initialize the state and have a default impl. for pre_spawn but requiring the Default bound on some states, makes it unnecessarily complex for states that will always be created without the need of a default.

Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

For me currently this seems like it adds more to the 'framework' which needs to be supported by each individual service, but does not add functionality to the framework which makes creating and managing services easier.

So it has a bit the opposite effect, it puts more burden on the implementation side, which is what we would ideally like to reduce. That's where the benefits come from (1 framework, but many services).

@iambriccardo
Copy link
Member Author

For me currently this seems like it adds more to the 'framework' which needs to be supported by each individual service, but does not add functionality to the framework which makes creating and managing services easier.

So it has a bit the opposite effect, it puts more burden on the implementation side, which is what we would ideally like to reduce. That's where the benefits come from (1 framework, but many services).

I fully agree.

In the service rewrite I attempted, I implemented an abstraction similar to a framework. Unfortunately, it didn’t align well with our specific service usage so I discarded it.

Rather than adding new functionality, this approach focuses on standardizing common service use cases through abstractions, nothing more.

If you have any proposals to do this better, more than happy to start a discussion.

@jjbayer
Copy link
Member

jjbayer commented Oct 31, 2024

For me currently this seems like it adds more to the 'framework' which needs to be supported by each individual service

I would only go through with this change if we can have sensible defaults, i.e.

  1. services that don't need a public state automatically get () somehow (not sure if this is possible without the associated type defaults that @loewenheim mentioned),
  2. default impl of pre_spawn,
  3. .start() still returns an Addr as before. Offer an additional start_observable() if the client of the service actually needs to observe it.

Passing the shutdown handle into spawn_handler might be a good idea independent of the other changes, also for testability.

@iambriccardo
Copy link
Member Author

For me currently this seems like it adds more to the 'framework' which needs to be supported by each individual service

I would only go through with this change if we can have sensible defaults, i.e.

  1. services that don't need a public state automatically get () somehow (not sure if this is possible without the associated type defaults that @loewenheim mentioned),
  2. default impl of pre_spawn,
  3. .start() still returns an Addr as before. Offer an additional start_observable() if the client of the service actually needs to observe it.

Passing the shutdown handle into spawn_handler might be a good idea independent of the other changes, also for testability.

I can make all of such changes but the Default one is not ergonomic, since it forces states to have defaults even when they are not needed.

@Dav1dde
Copy link
Member

Dav1dde commented Oct 31, 2024

Rather than adding new functionality, this approach focuses on standardizing common service use cases through abstractions, nothing more.

I don't see the upside of this at the moment. More abstractions should also take over logic/complexity of the individual services. For example, instead of passing down a shutdown handle (which could also just be another argument to the constructor), could the framework manage graceful shutdowns including dependencies.

@jan-auer jan-auer self-requested a review October 31, 2024 08:39
@iambriccardo
Copy link
Member Author

iambriccardo commented Oct 31, 2024

Rather than adding new functionality, this approach focuses on standardizing common service use cases through abstractions, nothing more.

I don't see the upside of this at the moment. More abstractions should also take over logic/complexity of the individual services. For example, instead of passing down a shutdown handle (which could also just be another argument to the constructor), could the framework manage graceful shutdowns including dependencies.

I explored this. The problem, which I might need help with is that we need to have all the code in a top-level select in the loop which polls from multiple futures and you can't fuse selects (from what I saw). Another mechanism would be to handle the shutdown by spawning a separate task for each service which is somehow bound to the joinhandle of the main task and kills it (if it's possible).

@Dav1dde
Copy link
Member

Dav1dde commented Oct 31, 2024

There is always the option of not doing something if the alternative does not achieve what was hoped.

@iambriccardo
Copy link
Member Author

There is always the option of not doing something if the alternative does not achieve what was hoped.

Indeed, this is the reason why we are discussing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SPIKE] Explore possible advancements of the Service framework
4 participants