Migrate Logger away from GenEvent

[josevalim]

The goal is to migrate Logger handlers away from GenEvent. We can either use Erlang's gen_event or have a custom implementation on top of GenServer. We are not going to use GenStage because this is one of the few cases where gen_event's architecture actually makes sense: we want to avoid copying between multiple handlers. Therefore, differently from #5254, this issue can be tackled now-ish.

[ericentin]

@josevalim I'd like to take a crack at this unless you have someone else in mind?

[josevalim]

@antipax that would be fantastic, please go ahead. I would start with gen_event at first. Keep in mind ExUnit kinda depends on Logger internals.

[ericentin]

So here's a couple things that have come up that we should discuss:

• GenEvent provides an error when you try to add a duplicate handler to a manager. :gen_event provides none.
• GenEvent provides add_mon_handler, which is impossible to implement with the :gen_event manager

A possible solution to the first is to allow duplicate backends to be installed, or, we could track them separately somewhere, like an ETS table or another process.

As for the second, I think we could use add_sup_handler, because currently the process which uses add_mon_handler exits in all the cases it would with add_sup_handler, AFAIK.

[josevalim]

Yes, we don't need the mon handler feature because we can use the sup handler.

For the uniqueness thing though, maybe we can use the logger watcher supervisor to give us uniqueness, as it requires the watchers id to be unique. This means we can't use a simple one for one though (which is fine, we wouldn't have dozens of children anyway).

José Valimwww.plataformatec.com.br http://www.plataformatec.com.br/Founder and Director of R&D

[ericentin]

So, I don't think we can use a :one_for_one supervisor for this. The child spec for a :transient child persists in the supervisor even after the process dies, so you cannot remove and then re-add a child with a given ID. Any thoughts? Could just use a separate gatekeeper process or ETS table.

[josevalim]

I can think of two options:

• allow duplicates and leave it up to the user to handle those with care?
• have an ets table and have the watcher check the table on init, maybe returning ignore in case of already taken

/cc @fishcakez

[fishcakez]

Can we pass this to the calling process? The watcher can be removed with delete_child if it is present and keeps the uniqueness guarantee in a single place.

def start_backend(module, arg) do
  case Supervisor.start_child(WatcherSup, worker(Watcher, [module, arg], [id: module])) do
    {:error, :already_present} ->
      _ = Supervisor.delete_child(WatcherSup, module)
      start_backend(module, arg)
   other ->
    other
end

[ericentin]

Yes, that's a good point. I had not thought to use :already_present vs. :already_started for this purpose.

Could there be a (perhaps unimportant) race where two processes attempt to add a backend, and instead of it getting added once, it gets added, removed, and added again?

[fishcakez]

Its not possible because delete_child does not work unless the child has undefined pid. So the second client in the race will get an already started error.

On Tuesday, 27 September 2016, Eric Entin notifications@github.com wrote:

Yes, that's a good point. I had not thought to use :already_present vs. :already_started for this purpose.

Could there be a (perhaps unimportant) race where two processes attempt to add a backend, and instead of it getting added once, it gets added, removed, and added again?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/elixir-lang/elixir/issues/5255#issuecomment-249914625, or mute the thread https://github.com/notifications/unsubscribe-auth/AB6JTUh0SscKxJ9Nx09A6Bt7HinvyGIIks5quUCRgaJpZM4KGS_W .

[ericentin]

Perfect! Thanks @fishcakez! ❤️

I'll move ahead with this approach.