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

Implementing Erlang supervisor-based dynamic supervisor #87

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

guillheu
Copy link

@guillheu guillheu commented Nov 23, 2024

Implementing #42
Following lpil' recommendation, I started from the static_supervisor module, which uses Erlang's supervisor module. I added bindings to the following Erlang supervisor functions:

  • start_child/2
  • terminate_child/2
  • restart_child/2
  • delete_child/2
  • count_children/1

I also added a new supervisor type, SimpleSupervisor, which implements the simple-one-for-one strategy. It needed its own type, because it behaves significantly differently with the aforementioned functions than the other strategies. In my opinion, they are different kinds of supervisors altogether.
As a result, the Supervisor can be managed with functions like add_child, terminate_child and count_children, while the equivalent functions for the SimpleSupervisor are prefixed with simple_ (simple_add_child, simple_terminate_child, simple_count_children) to have them clearly stand out and stick together in the hex function index.

I named the new module erlang_supervisor. Since my inspiration was static_supervisor, anyone using the static_supervisor can use the new erlang_supervisor instead. As a result, I took the initiative to deprecate all the functions in the static_supervisor module (apologies if that's overreach on my part).

Because of how different the Supervisor and SimpleSupervisor are, I think there's an argument to split them into separate modules. However they would be using the same Erlang bindings under the hood, so there'd have to be a third internal module for the bindings and other common logic. I wasn't sure how much should be in that internal module and how much should stay exposed, so I didn't split them.

Finally, I updated all the tests of the existing strategies with the new functionalities, and wrote a new test for the simple-one-for-one strategy.

I think some things could be improved, like the error types for the Erlang functions.

EDIT:
Since I first opened this PR, I learned about phantom types. Now, the Supervisor and Builder types are phantom types, to distinguish the "simple" supervisors from the "classic" ones. This is still necessary, since the "Simple" supervisor (which only implements the simple-one-for-one strategy) and the "Classic" supervisor (all 3 remaining strategies) have similar but not identical behaviors.
I also changed the supervisor_child function to be built from a Builder while still returning a ChildSpec, making it easy to build a supervision tree, both when starting the whole tree in one go or when progressively adding supervisor children with start_child.

…aves differently from the others, and one for the others.
…separate types and functions for the "simple" supervisors. Deprecating `static_supervisor`.
@lpil
Copy link
Member

lpil commented Dec 22, 2024

Hello! The issue was to add dynamic-supervisor, but this seems to be flexible bindings to the Erlang supervisor module in general, but we have a slightly more specialised version of that already, static_supervisor.

It seems to me that this combined API is a lot more challenging to understand than the split design proposed and also used in Elixir. What do you see as the advantages in exchange for that drawback?

Thank you

@guillheu
Copy link
Author

guillheu commented Dec 22, 2024

There doesn't seem to be any meaningful difference between a "static" or "dynamic" supervisors in Erlang. Instead, it's the children that are defined statically or dynamically, but either way they're following the exact same child spec.
This means an Erlang supervisor can both be started with statically-defined children, and then have children added to it dynamically.
In my eyes, that flexibility matters more than simplifying the API. Separate "static" and "dynamic" supervisor types would create an arbitrary and unnecessary constraint, that the user can only either define child processes statically, or dynamically, but not both, for a given supervisor. There is nothing from the Erlang supervisor that imposes that distinction.

That being said, I think it's possible to clarify the API while keeping that supervisor flexibility.

I think it's fair to criticize the API. When I first built this I was mostly worried with expanding the feature set of the existing Erlang static supervisor. That being said, my main concern is to only have a single Supervisor type that would work both with statically and dynamically defined children.

What do you think of having a gleam/otp/supervisor/erlang module with Builder, Supervisor and ChildBuilder types, and gleam/otp/supervisor/erlang/dynamic and gleam/otp/supervisor/erlang/static modules, each implementing functions for statically (add(Builder, ChildBuilder...)) and dynamically (start_child(Supervisor, ChildBuilder...)) defined children respectively ? Possibly with shorter module names.

@lpil
Copy link
Member

lpil commented Dec 23, 2024

I disagree! They are different actor abstractions that fulfil different tasks. The fact that the API is almost perfectly split into two different set of functions using phantom types and two different start functions is evidence of this. Very little in this module is shared between both concepts, and we have good evidence from Elixir that splitting them into separate things makes it much easier to understand. That way what it does is clearer, and we don't have to explain on each function or type which one it is relevant to.

What do you think of having a gleam/otp/supervisor/erlang module with Builder, Supervisor and ChildBuilder types

I think it would be better to design optimal APIs for each rather than assume they will be the same. Worse-case-scenario it's a small amount of duplication, and we have no problem with duplication in Gleam.

@guillheu
Copy link
Author

guillheu commented Dec 23, 2024

The fact that the API is almost perfectly split into two different set of functions using phantom types and two different start functions is evidence of this. Very little in this module is shared between both concepts

The phantom type on the Supervisor is not to distinguish dynamic and static supervisors, but to distinguish "simple" and... "non-simple" (I call them "classic") supervisors. Those have a decent amount of overlap, but some functions differ between the two. I don't really know anything about Elixir, but AFAIK it uses dynamic types, same as Erlang, which is what allows "simple" and "classic" supervisors to share the same functions while behaving very differently.

Both "simple" and "classic" supervisors can have dynamic and static children.
The phantom type is not a distinction between static and dynamic supervisors. There really isn't any technical difference.

Also, it's not duplication I have a problem with, it's flexibility. Having separate types for "dynamic" and "static" supervisors does create the restriction I mentioned in my previous comment, that one wouldn't be able to both add children statically, and then add more children dynamically. There's always workarounds, but then that's not really making the library easier to use, which is the goal.

Again, I agree that the API should be split, but not by having distinct supervisor types.

However I also noticed something. The "static" children are added via the Builder type, which, when started, returns a Supervisor type. That Supervisor type can then have children added dynamically. So in a sense, the functions for "static" children could be defined in one module with the Builder type, while the functions for "dynamic" children could be defined in another module with the Supervisor type.
Let's imagine having a static and dynamic modules. The static.start_link function would take in a static.Builder argument and return a dynamic.Supervisor which could then be used with dynamic.start_child etc...
And for the sake of clarity and completeness, the dynamic module would also have a dynamic.start_link function but which wouldn't take a full builder, just some supervisor options, and return a dynamic.Supervisor with no children.

Something like this:
static.gleam

type Builder(kind) {
...
}

pub fn start_link(builder: Builder(kind), ...) -> dynamic.Supervisor(kind) {...}

dynamic.gleam

type Supervisor(kind) {
...
}

pub fn start_link(options: ...) -> Supervisor(kind) {...}

pub fn start_child(supervisor: Supervisor(kind), ...) -> Supervisor(kind) {...}

There would still be more to do about the "simple" and "classic" supervisors but maybe this would split the API in a way that's acceptable to you ? One that still follows the Elixir example without adding unnecessary restrictions ?

EDIT:
Another thought occurred to me, that maybe Elixir can create that distinction between static and dynamic supervisors because of dynamic typing allowing to create a static supervisor and then add children dynamically to it ? I don't actually know, just a thought, but if that's a case then that explains why we can't just copy what they do without significant changes or without loss of flexibility.

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.

2 participants