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

Re-architecture of Alice's supervision tree structure #85

Draft
wants to merge 2 commits into
base: a2
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions lib/alice.ex
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,19 @@ defmodule Alice do
end

@doc """
Stops an Alice bot instance
Stops an Alice bot instance along with its handlers and adapters
"""
def stop_bot(pid) do
Supervisor.terminate_child(Alice.Bot.Supervisor, pid)
if Process.alive?(pid) do
%{adapters: adapters, handlers: handlers} = :sys.get_state(pid)
for adapter <- adapters do
Supervisor.terminate_child(Alice.Adapter.Supervisor, Process.whereis(adapter))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could Supervisor.stop (docs here) with Alice.Adapter.Supervisorbe used to avoid the loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look into it, that would be much better. This is what I had to do to get rid of that error messages after the tests run, and I'm all for simplifying it.

end
for handler <- handlers do
Supervisor.terminate_child(Alice.Handler.Supervisor, Process.whereis(handler))
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question here for the handler supervisor.

end
Supervisor.terminate_child(Alice.Bot.Supervisor, pid)
end
end

@doc """
Expand Down
20 changes: 5 additions & 15 deletions lib/alice/adapter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ defmodule Alice.Adapter do
@callback reply(bot, msg) :: term

@doc false
def start_link(module, opts) do
GenServer.start_link(module, {self(), opts})
def start_link(adapter_module, bot_pid, opts) do
GenServer.start_link(adapter_module, {bot_pid, opts})
end

@doc false
Expand All @@ -25,13 +25,8 @@ defmodule Alice.Adapter do
use GenServer
@behaviour Alice.Adapter

def reply(bot, %Alice.Message{} = msg) do
GenServer.cast(bot, {:reply, msg})
end

@doc false
def start_link(bot, opts) do
Alice.Adapter.start_link(__MODULE__, opts)
def reply(pid, %Alice.Message{} = msg) do
GenServer.cast(pid, {:reply, msg})
end

@doc false
Expand All @@ -46,12 +41,7 @@ defmodule Alice.Adapter do
:ok
end

@doc false
defmacro __before_compile__(_env) do
:ok
end

defoverridable [__before_compile__: 1, reply: 2]
defoverridable [reply: 2]
end
end
end
15 changes: 15 additions & 0 deletions lib/alice/adapter/supervisor.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
defmodule Alice.Adapter.Supervisor do
@moduledoc """
Supervises any Alice.Adapter process that are started.
Copy link
Contributor

Choose a reason for hiding this comment

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

processes instead of process. 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

"""

use Supervisor

def start_link(_) do
Supervisor.start_link([
worker(Alice.Adapter, [], restart: :transient)
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to consider converting this to a module based supervisor, [per the docs here] (https://hexdocs.pm/elixir/Supervisor.html#module-module-based-supervisors) particularly since this is a simple_one_for_one supervisor. This will be important in particular if we want to support hot code-reloading as a deployment strategy (and I think we do). This change seems pretty simple, as it means moving most things to the init/1 callback rather than start_link/2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I wasn't doing anything in init for now I used the simplified version, but I think you're right. It will leave it more open for change and doesn't add much, if any, extra complexity.

], strategy: :simple_one_for_one, name: __MODULE__)
end

def init(_), do: :ok
end
6 changes: 3 additions & 3 deletions lib/alice/adapters/console.ex
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ defmodule Alice.Adapters.Console do
ref: make_ref(),
bot: bot,
text: text,
type: "chat",
room: "console",
user: user
adapter: {__MODULE__, self()},
type: "chat",
user: %Alice.User{id: "console", name: user}
}
end
end

9 changes: 7 additions & 2 deletions lib/alice/adapters/noop.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,19 @@ defmodule Alice.Adapters.NoOp do
end

@doc false
def handle_cast({:reply, _msg}, bot) do
def handle_cast({:reply, msg}, bot) do
IO.puts "got reply message #{inspect msg} from bot #{inspect bot}"
{:noreply, bot}
end

@doc false
def handle_info(:connected, bot) do
:ok = Alice.Bot.handle_connect(bot)
IO.puts "NoOp connected"
{:noreply, bot}
end
def handle_info({:message, msg}, bot) do
IO.puts "received message #{inspect msg} for bot #{inspect bot}"
Alice.Bot.handle_in(bot, make_msg(bot, msg))
{:noreply, bot}
end
Expand All @@ -39,8 +42,10 @@ defmodule Alice.Adapters.NoOp do
ref: make_ref(),
bot: bot,
text: msg,
room: "noop",
adapter: {__MODULE__, self()},
type: "chat",
user: "user"
user: %Alice.User{id: "user", name: "user"}
}
end
end
2 changes: 1 addition & 1 deletion lib/alice/adapters/test.ex
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ defmodule Alice.Adapters.Test do
end

def handle_info({:message, msg}, %{bot: bot} = state) do
msg = %Alice.Message{bot: bot, text: msg.text, user: msg.user}
msg = %Alice.Message{bot: bot, adapter: {__MODULE__, self()}, text: msg.text, user: msg.user}
Alice.Bot.handle_in(bot, msg)
{:noreply, state}
end
Expand Down
63 changes: 42 additions & 21 deletions lib/alice/bot.ex
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,16 @@ defmodule Alice.Bot do
*`handers` - list of handlers to register
"""

defstruct adapter: nil,
name: nil,
defstruct name: nil,
adapters: [],
handlers: [],
handler_sup: [],
opts: []

def new(adapter, name, handlers, handler_sup, opts) do
def new(name, adapters, handlers, opts) do
%__MODULE__{
adapter: adapter,
name: name,
adapters: adapters,
handlers: handlers,
handler_sup: handler_sup,
opts: opts
}
end
Expand Down Expand Up @@ -106,8 +104,7 @@ defmodule Alice.Bot do

config = Alice.Bot.Config.init_config(__MODULE__, opts)

@adapter config.adapter
@before_compile config.adapter
@adapters config.adapters
@config config.bot_config
@log_level config.bot_config[:log_level]
@otp_app config.otp_app
Expand All @@ -134,14 +131,13 @@ defmodule Alice.Bot do
Logger.unquote(@log_level)(fn()-> msg end, [])
end

def __adapter__, do: @adapter
def __adapters__, do: @adapters

def init({bot_module, opts}) do
with {handlers, opts} <- register_handlers(bot_module.bot_config(opts)),
{:ok, adapter} <- @adapter.start_link(bot_module, opts),
{:ok, handler_sup} <- Alice.Handler.Supervisor.start_link(),
{adapters, opts} <- register_adapters(opts),
name <- opts[:name],
bot <- Alice.Bot.new(adapter, name, handlers, handler_sup, opts) do
bot <- Alice.Bot.new(name, adapters, handlers, opts) do
{:ok, bot}
else
_ -> {:stop, :shutdown}
Expand All @@ -154,6 +150,12 @@ defmodule Alice.Bot do
{handlers, opts}
end

defp register_adapters(opts) do
{adapters, opts} = Keyword.pop(opts, :adapters, [])
GenServer.cast(self(), {:register_adapters, adapters, opts})
{adapters, opts}
end

def handle_connect(state) do
{:ok, state}
end
Expand All @@ -172,9 +174,9 @@ defmodule Alice.Bot do
def handle_call(:name, _from, %{name: name} = state) do
{:reply, name, state}
end
def handle_call(:handler_processes, _from, %{handler_sup: sup} = state) do
def handle_call(:handler_processes, _from, state) do
handler_processes =
sup
Alice.Handler.Supervisor
|> Supervisor.which_children()
|> Enum.map(fn({_,pid,_,_}) ->
{_,[{_,{mod,_,_}}|_]} = Process.info(pid, :dictionary)
Expand All @@ -199,14 +201,15 @@ defmodule Alice.Bot do
end
end

def handle_cast({:reply, msg}, %{adapter: adapter} = state) do
@adapter.reply(adapter, msg)
def handle_cast({:reply, msg}, state) do
{adapter_mod, adapter_pid} = msg.adapter
adapter_mod.reply(adapter_pid, msg)
{:noreply, state}
end
def handle_cast({:handle_in, msg}, state) do
case handle_in(msg, state) do
{:dispatch, %Alice.Message{} = msg, state} ->
handlers = Supervisor.which_children(state.handler_sup)
handlers = Supervisor.which_children(Alice.Handler.Supervisor)
Alice.Handler.dispatch(msg, handlers)
{:noreply, state}

Expand All @@ -225,13 +228,31 @@ defmodule Alice.Bot do
{:noreply, state} -> {:noreply, state}
end
end
def handle_cast({:register_handlers, handlers}, %{name: name, handler_sup: sup} = state) do
def handle_cast({:register_handlers, handlers}, %{name: name} = state) do
handlers = ensure_builtin_handlers(handlers)
Enum.each(handlers, fn(handler) ->
Supervisor.start_child(sup, [handler, {name, self()}])
end)
for handler <- handlers do
{:ok, pid} = Supervisor.start_child(Alice.Handler.Supervisor, [handler, {name, self()}])
ProcessUtils.register_eventually(pid, handler)
end
{:noreply, %{state | handlers: handlers}}
end
def handle_cast({:register_adapters, adapters, opts}, state) do
adapters = for adapter <- ensure_adapter(adapters) do
case adapter do
{name, mod} ->
{:ok, pid} = Supervisor.start_child(Alice.Adapter.Supervisor, [mod, self(), opts])
ProcessUtils.register_eventually(pid, name)
mod
mod when is_atom(mod) ->
{:ok, _pid} = Supervisor.start_child(Alice.Adapter.Supervisor, [mod, self(), opts])
mod
end
end
{:noreply, %{state | adapters: adapters}}
end

defp ensure_adapter([]), do: [{:default_noop, Alice.Adapters.NoOp}]
defp ensure_adapter(adapters), do: adapters

defp ensure_builtin_handlers(handlers) when is_list(handlers) do
Enum.reduce(Alice.Handler.builtins(), handlers, fn(builtin, acc) ->
Expand Down
22 changes: 12 additions & 10 deletions lib/alice/bot/config.ex
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
defmodule Alice.Bot.Config do
defstruct [:otp_app, :adapter, :bot_config]
defstruct [:otp_app, :adapters, :bot_config]

def new(otp_app, adapter, bot_config) do
%__MODULE__{otp_app: otp_app, adapter: adapter, bot_config: bot_config}
def new(otp_app, adapters, bot_config) do
%__MODULE__{otp_app: otp_app, adapters: adapters, bot_config: bot_config}
end

def init_config(bot_module, opts_from_use) do
otp_app = opts_from_use[:otp_app]
bot_config = get_bot_config(bot_module, otp_app, opts_from_use)
adapter = ensure_adapter!(opts_from_use, bot_config)
new(otp_app, adapter, bot_config)
adapters = get_adapters(opts_from_use, bot_config)
new(otp_app, adapters, bot_config)
end

def get_bot_config(bot_module, otp_app, opts_from_use) do
Expand All @@ -34,10 +34,12 @@ defmodule Alice.Bot.Config do
|> Keyword.merge(opts_from_use)
end

defp ensure_adapter!(opts, config) do
adapter = opts[:adapter] || config[:adapter] || :no_adapter
ensure_adapter!(adapter)
defp get_adapters(opts, config) do
with [] <- opts[:adapters],
[] <- config[:adapters] do
[]
else
adapters -> adapters
end
end
defp ensure_adapter!(:no_adapter), do: raise ArgumentError, "please configure an adapter"
defp ensure_adapter!(adapter), do: adapter
end
15 changes: 8 additions & 7 deletions lib/alice/bot/supervisor.ex
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
defmodule Alice.Bot.Supervisor do
@moduledoc false
@moduledoc """
Supervises any Alice.Bot process that are started.
"""

use Supervisor

def start_link(opts \\ []) do
Supervisor.start_link(__MODULE__, :ok, opts)
def start_link(_) do
Supervisor.start_link([
worker(Alice.Bot, [], restart: :transient)
], strategy: :simple_one_for_one, name: __MODULE__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here about going with a module based supervisor. I also don't think we need a simple_one_for_one supervisor here. We may have discussed why a straight one_to_one wouldn't work, but i don't remember why that is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it SOFO because the bot process is started by the user's bot project after the entire Alice tree starts. Using SOFO, we don't have to double-supervise the bot process.

Copy link
Member Author

Choose a reason for hiding this comment

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

However, having said that, I'm not 100% happy with having to start the bot there kind of manually, so maybe that's something we still have to figure out.

end

def init(:ok) do
[worker(Alice.Bot, [], restart: :transient)]
|> supervise(strategy: :simple_one_for_one)
end
def init(_), do: :ok
end
15 changes: 10 additions & 5 deletions lib/alice/handler/supervisor.ex
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
defmodule Alice.Handler.Supervisor do
def start_link do
import Supervisor.Spec, warn: false
@moduledoc """
Supervises any Alice.Handler process that are started.
"""

children = [
use Supervisor

def start_link(_) do
Supervisor.start_link([
worker(Alice.Handler, [], restart: :transient)
]
Supervisor.start_link(children, strategy: :simple_one_for_one)
], strategy: :simple_one_for_one, name: __MODULE__)
end

def init(_), do: :ok
end
19 changes: 11 additions & 8 deletions lib/alice/message.ex
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
defmodule Alice.Message do
@moduledoc "Alice message"

@type captures :: list | map
@type private :: map
@type ref :: reference
@type bot :: pid
@type room :: binary
@type text :: binary
@type type :: binary
@type user :: Alice.User.t
@type captures :: map()
@type private :: map()
@type ref :: reference()
@type bot :: pid()
@type adapter :: {module(), pid()}
@type room :: binary()
@type text :: binary()
@type type :: binary()
@type user :: Alice.User.t()

@type t :: %__MODULE__{
captures: captures,
private: private,
ref: ref,
bot: bot,
adapter: adapter,
room: room,
text: text,
type: type,
Expand All @@ -25,6 +27,7 @@ defmodule Alice.Message do
private: %{},
ref: nil,
bot: nil,
adapter: nil,
room: nil,
text: nil,
type: nil,
Expand Down
Loading