Custom event loop

[Restioson]

This is a POC for custom event loops. I purposefully have not addressed #91 in this draft, as this is simply the minimal set of changes that provide custom event loops, and may serve as a jumping-off point for a PR to address #91. This is a spiritual successor to #51 in a way 😄

Closes #89

[Restioson]

CC @thomaseizinger, for some reason it won't let me request a review from anyone?

[thomaseizinger]

CC @thomaseizinger, for some reason it won't let me request a review from anyone?

That is because I am not listed as a maintainer, I think :)

[thomaseizinger]

Cool!

Thanks for giving this a go! :)
Left a few comments!

[Restioson]

Ahh, I see. I can add you as a collaborator if you'd like?

Also, do you think it makes sense to tighten up this PR and get it merged soon, and after that tackle modularisation?

[thomaseizinger]

Ahh, I see. I can add you as a collaborator if you'd like?

Sure! Much appreciated :)

Also, do you think it makes sense to tighten up this PR and get it merged soon, and after that tackle modularisation?

Yes to both.

[Restioson]

One last thing to be done here: I think now that the future from tick can be cancelled and therefore the catty::Sender dropped without the actor stopping, mapping it to Disconnected doesn't really make sense. Something like Interrupted would make more sense. Maybe an error enum could be made:

enum Error {
    ActorShutdown,
    Disconnected,
    Interrupted,
}

We could also tackle #89 here and maybe remove the ActorShutdown if it's unnecessary.

A possible usecase for how senders could interpret Interrupted as differently from Disconnected would be if they had handler timeouts and wanted to check if the message send failed due to timeout (Interrupted) or the actor being stopped (Disconnected).

[thomaseizinger]

Nice!

[Restioson]

I'll merge this after I make CI pass

EDIT: holding off on merge until the latest change has been reviewed

[Restioson]

One last thing to be done here: I think now that the future from tick can be cancelled and therefore the catty::Sender dropped without the actor stopping, mapping it to Disconnected doesn't really make sense. Something like Interrupted would make more sense

I've added a new Error enum to address this now. In doing so, I also removed ActorShutdown, so this PR would close #89 too. It's not technically so well scoped but it felt like a good time since Error needed to be made into an enum to support the dual possible causes of send failure.

[thomaseizinger]

Looks good apart from the removal of -> Self::Stopped :)

[thomaseizinger]

Can we include the actor name via std::any::type_name in here?

[thomaseizinger]

We would need a constructor for this to avoid the type parameter:

impl Error {
	pub fn disconnected::<A: Any>() - Self {
		Self::Disconnected(std::any::type_name::<A>())
	}
}

[thomaseizinger]

For encapsulation purposes, we could make this enum private and encapsulating it in an opaque error, similar to std::io::Error but we'd lose exhaustive matching then because users can only check of the types via Error::is_interrupted() etc.

[Restioson]

I think that having exhaustive matching is probably useful, so I would rather leave it as it is now, unless there is a compelling reason to encapsulate it

[Restioson]

Also, should the interrupted variant also have the name?

[thomaseizinger]

Also, should the interrupted variant also have the name?

I guess so? The name is useful because this error may often be question-marked and then appear in an error chain from anyhow or something. Without the actor name, it is really hard to debug where it came from.

[Restioson]

In that case, I wonder if it should be extracted as a field:

struct Error {
    act: &'static str,
    kind: ErrorKind,
}

with the current Error renamed to ErrorKind

[Restioson]

I've been working on this and it's kinda outgrown this PR, so would you be ok with merging this now and having this feature in a followup? I've pushed my work on that to custom-event-loop-error

[Restioson]

Strange... the line the CI is failing on doesn't exist??

[thomaseizinger]

Strange... the line the CI is failing on doesn't exist??

Merge with master. The CI is running on pull_request meaning it is running on a merge commit with latest master.

[Restioson]

Ah, I see!