Provide watcher handlers in the GenEvent module

[josevalim]

Basically the idea is: should we provide the watcher handlers functionality as part of GenEvent?

If so, we need to APIs, one that starts the GenEvent handler along side its GenServer to be embedded as part of a supervision tree and another that starts it directly in a given supervision tree. I thought about the name add_watched_handler/3 (I would prefer add_sup_handler/3 but that would cause confusion with Erlang's) and have the :sup option when you want to embed it on a particular supervisor.

/cc @fishcakez

[fishcakez]

I am struggling to think of the appropriate level of abstraction.

To me most suitable level is to provide a normal OTP process:

defmodule GenEvent.Watcher do

  @spec start_link(pid() | atom() | {:global, any} | {:via, module, any}, module | {module, any}, any, GenServer.options) ::
    {:ok, pid} | :ignore | {:error, any}
  def start_link(gen_event, handler, args, opts \\ []) do
    GenServer.start_link(__MODULE__, [gen_event, handler, args], opts)
  end

This allows putting the watcher process in any point of a supervision tree, including the childspecs of a supervisor:

worker(GenEvent.Watcher, [gen_event, handler, args)

Is this too low level?

[josevalim]

That's exactly what I had in mind except the API lives in
GenEvent.add_watched_handler instead of a separated module.

One question that affects the design is if we want to provide a default
supervisor with Elixir or ask people to define their own.

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Lead Developer

[fishcakez]

I don't think their is any benefit to providing a Supervisor because Supervisor.Default makes declaring a callback-less supervisor so easy/concise.

I prefer using a separate module. I am always keen to follow one OTP process - one module. This is likely a hangover from Erlang practises but I find it makes it easier to find and follow things. Normally I can't back this up with anything because it is a matter of personal taste. However in this case I think the way childspecs would have to be written shows it should have its own module:

worker(GenEvent, [gen_event, handler, args], [function: add_watched_handler, modules: [GenEvent.Watcher])

I also feel that start_link is a more accurate description that add_watched_handler. start_link is standard and I feel add_watched_handler is too similar to add_handler but behaves quite differently.

[josevalim]

Ok, I am sold on GenEvent.Watcher at least for the ones added directly to the supervision tree. What about when I want to attach it into an existing supervisor? What if we have two APIs:

1. GenEvent.Watcher.start_link/4 - for cases when you want to hook a watcher directly in a supervision tree
2. GenEvent.add_sup_handler(supervisor, manager, handler, args, opts \\ []) for when you want to dynamically add it to a supervisor

For 2, we could also use add_handler but since having a supervisor changes the return type, it is better to have its own function.

Finally, for the supervised case, what is the best approach if the handler is already in the GenEvent? GenEvent.add_handler/2 can add the same handler multiple times but in the supervised case, because we need an ID, it cannot be added multiple times, thoughts?

I could also be overthinking and maybe we don't need to provide the supervised version (it may be easy enough to roll your own once we already provide the watcher).

[fishcakez]

If we pretend we have only implemented GenEvent.Watcher.start_link/4, lets see what we would do:

import Supervisor.spec
Supervisor.start_child(sup, worker(GenEvent.Watcher, [gen_event, handler, args])

Is abit too verbose. Maybe we can provide a childspec function:

# id = {GenEvent.Watcher, gen_event, handler} ?
Supervisor.start_child(sup, GenEvent.Watcher.childspec(gen_event, handler, args))

That doesn't really help at all, we probably do need a helper function. I usually create a helper function in the supervisor's module for starting its dynamic children. I call them start_x, where x describes the child:

GenEvent.start_watcher(sup, gen_event, handler, args)

I feel this is clearer than add_sup_handler because it conveys what is going on better and is similar to Supervisor.start_child/2 so feels natural. I don't know, its up to you.

The multiple handler case can be handled by using id's in the childspec or registering the watchers.

[josevalim]

You are right, let's just provide a GenEvent.Watcher for now and we can always revisit later. Baby steps. :) It will be after 1.0 though.

[josevalim]

One of the things that came to me is that this is much easier with streams. All you need to do to have a supervised task and consume the GenEvent as a stream in the task.

[lexmag]

I've found myself a couple of times reimplementing Logger.Watcher; are there any plans to have the GenEvent.Watcher? I'd like to work on it if it's needed.

[josevalim]

This is closed because the plan is to get rid of GenEvent in the long term.