feat(actix): Generic Addr type #1405

flub · 2022-08-10T14:57:23Z

This makes Addr generic including the return value, just for the
Healthcheck actor for now.

The HealthcheckMessage has been renamed to HealthcheckEnvelope and
still needs to be a manual list of all the messages the actor
implements. The conversion from messages to this is now a little more
involved that an impl From, since this also encodes the return type
so you need to type this out with the ActorMessage trait impl.

Sending to the actors is now generic, and the actors main loop also
remains reasonable clean to implement.

What hasn't been done yet is extracting this out to proper shared
locations etc.

Follow up tasks

Move this into relay-system in a new module.
Benchmark whether we should box the envelopes by default.

Go home danger, you're drunk:
#skip-changelog

This makes `Addr` generic including the return value, just for the Healthcheck actor for now. The `HealthcheckMessage` has been renamed to `HealthcheckEnvelope` and still needs to be a manual list of all the messages the actor implements. The conversion from messages to this is now a little more involved that an `impl From`, since this also encodes the return type so you need to type this out with the `ActorMessage` trait impl. Sending to the actors is now generic, and the actors main loop also remains reasonable clean to implement.

This is a bit more readable. It also generally renames Actor -> Service since apparently we want to avoid the Actor name for some reason and align us more the Tower services at some point in the future?

jan-auer

This is a great way to model our design principles, and I'm +1 on going forward with this once we have a few more examples to evaluate this on. It's interesting how close this gets the signatures to actix again.

I left a few early comments below on the current state, to start some discussions.

relay-server/src/endpoints/healthcheck.rs

relay-server/src/actors/healthcheck.rs

jan-auer · 2022-08-12T06:28:32Z

relay-server/src/actors/healthcheck.rs

+pub trait ServiceMessage<S: Service> {
+    type Response;
+
+    fn into_envelope(self) -> (S::Envelope, oneshot::Receiver<Self::Response>);


We will have a good amount of messages that are fire-and-forget, so they do not need a oneshot receiver. Could we make the receiver optional?

The safest way would even be to type this as associated type in the service message, so that we can enforce two kinds of Addr::send methods via the type system.

I'm not sure how to approach this with another associated type, the simplest would be to create an entire new trait like "OnewayServiceMessage" or so. Though I'm also not really sure how much we care. We can just let those messages return () and not await them (we can abstract this behind do_send sure). That way you still always have the option of checking a message was actually received (I'm no fan of just swallowing the fact that the receiver might be gone, seems like a huge bug and maybe a panic would be better)

relay-server/src/actors/healthcheck.rs

jan-auer · 2022-08-12T06:46:41Z

relay-server/src/actors/healthcheck.rs

-                        HealthcheckMessage::Health(data) => service.handle_is_healthy(data).await,
+                    match message {
+                        HealthcheckEnvelope::IsHealthy(msg, response_tx) => {
+                            let response = service.handle_is_healthy(msg).await;


We could now introduce something like ServiceMessage::handle(self, service: &S) which contains the async definition of the handler and returns the result. That would sadly require async_trait though. Also, I have no good answer to model the fire-and-forget case.

I'm going to leave this as potentially future work. I want to wait a bit longer until I see the patterns better.

jan-auer · 2022-08-12T06:53:07Z

relay-server/src/actors/healthcheck.rs

+pub enum HealthcheckEnvelope {
+    IsHealthy(IsHealthy, oneshot::Sender<bool>),
+}


Ideally, we benchmark whether it's faster to just generally box the messages rather than having an explicit enum type. I'm still very much in favor of having the enum like this, since that takes a lot of hidden magic away and makes it very clear what's happening.

My expectation is that boxing the messages will actually improve performance. We could box them internally here, though that will increase the boilerplate.

The simpler way requiring less boiler plate would be just having a trait object, however.

yep, this is a followup task

tobias-wilfert

For what it is worth, looks good to me.

jjbayer · 2022-08-16T08:26:30Z

relay-server/src/actors/healthcheck.rs

+    ///
+    /// It is an enum of all the message types that can be handled by this service together
+    /// with the response [sender](oneshot::Sender) for each message.
+    type Envelope: Send + 'static;


What was the design rationale behind having an enum, instead of making the message type a parameter of Service, and then implementing each message handler separately?

For example

struct MyStruct; impl Service<Shutdown> for MyStruct; impl Service<HealthCheck> for MyStruct;

This came out of the aim to be able to send messages without having to do any boxing to put things in the channel. Whether that's always desirable that's questionable, but e.g. I think for healthcheck that probably is beneficial. Really though this should be benchmarked, this version does still allow boxing everything by default quite easily should it turn out to be more performant.

I was initially interested in building both versions to compare, I'm still on the fence myself. But once this one existed no one disliked this enough to carry on with building the other version.

CHANGELOG.md

relay-server/src/actors/healthcheck.rs

- This PR updates the Store service to use the generic Addr introduced in #1405, to avoid duplicate code, it moves the shared code out to its own module in relay-system called service. - Some minor changes have been made to address the comments of Jan on #1397. ### Future Steps - With the generic Addr in place the time has come to make a new registry for the services, that replaces the hardcoded current solution.

With the generic Addr in place since #1405 this PR makes a new Register for the Services, which replaces the current hardcoded solution. Design choices: - The first approach was to use `Arc<Option<Register>>` however, this didn't work as `Arc::new` is not `const fn`. To avoid this `lazy_static` was used however that in turn didn't work as `DerefMut` was required and `lazy_static` only implements `Deref`. - A `OnceBox` is currently used for the Register which is initialised at the end of `ServiceState::start()` this avoids the need for any Options but also could cause issues in the future as the current registry is created at the beginning of the function and then gradually populated. A similar approach might need to be used in case of issues.

flub requested review from tobias-wilfert and jan-auer August 10, 2022 14:57

Tie the envelope together via an explicit Service trait

c6e0ee3

This is a bit more readable. It also generally renames Actor -> Service since apparently we want to avoid the Actor name for some reason and align us more the Tower services at some point in the future?

jan-auer reviewed Aug 12, 2022

View reviewed changes

remove unnecessary type hint

eb65394

flub force-pushed the flub/generic-addr-nobox branch from d68902c to eb65394 Compare August 12, 2022 08:21

Floris Bruynooghe added 5 commits August 12, 2022 10:24

Make our envelope and responses Send

4a15b3a

Better comment

d4f4a85

Avoid capturing &self in the returend future of Addr::send

3aeb52d

Merge branch 'master' into flub/generic-addr-nobox

3ed6936

Add changelog

19ed546

flub marked this pull request as ready for review August 12, 2022 10:09

flub requested a review from a team August 12, 2022 10:09

tobias-wilfert approved these changes Aug 12, 2022

View reviewed changes

tobias-wilfert mentioned this pull request Aug 16, 2022

ref(actix): Update Store Actor [INGEST-1565] #1415

Merged

jjbayer reviewed Aug 16, 2022

View reviewed changes

jan-auer approved these changes Aug 16, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

relay-server/src/actors/healthcheck.rs Outdated Show resolved Hide resolved

Floris Bruynooghe added 3 commits August 16, 2022 11:33

Slightly rewrite mapping to senderror

006566f

Merge branch 'master' into flub/generic-addr-nobox

12767b9

Fixup merge

d858a93

flub merged commit a01af0d into master Aug 16, 2022

flub deleted the flub/generic-addr-nobox branch August 16, 2022 13:24

This was referenced Aug 17, 2022

feat(actix): New Register [INGEST-1494] #1421

Merged

Future Proof Relay #1424

Closed

HazAT added this to the Upgrade Tokio in Relay milestone Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(actix): Generic Addr type #1405

feat(actix): Generic Addr type #1405

flub commented Aug 10, 2022 •

edited

Loading

jan-auer left a comment

jan-auer Aug 12, 2022

flub Aug 12, 2022

jan-auer Aug 12, 2022

flub Aug 12, 2022

jan-auer Aug 12, 2022

flub Aug 12, 2022

tobias-wilfert left a comment

jjbayer Aug 16, 2022

flub Aug 16, 2022

feat(actix): Generic Addr type #1405

feat(actix): Generic Addr type #1405

Conversation

flub commented Aug 10, 2022 • edited Loading

Follow up tasks

jan-auer left a 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

Choose a reason for hiding this comment

tobias-wilfert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flub commented Aug 10, 2022 •

edited

Loading