Fix logger to keep config through config_change/3

Store deleted handlers and live config data in ETS table

Author: fishcakez

lib/logger/lib/logger/app.ex

@@ -21,49 +21,59 @@ defmodule Logger.App do
                     {otp_reports?, sasl_reports?, threshold}, :link],
                   [id: Logger.ErrorHandler, function: :watcher])]
 
+    config = Logger.Config.new()
+
     case Supervisor.start_link(children, options) do
-      {:ok, _} = ok ->
-        deleted = delete_error_logger_handler(otp_reports?, :error_logger_tty_h, [])
-        deleted = delete_error_logger_handler(sasl_reports?, :sasl_report_tty_h, deleted)
-        store_deleted_handlers(deleted)
-        ok
+      {:ok, sup} ->
+        handlers = [error_logger_tty_h: otp_reports?,
+                    sasl_logger_tty_h: sasl_reports?]
+        delete_handlers(handlers)
+        {:ok, sup, config}
       {:error, _} = error ->
+        Logger.Config.delete(config)
         error
     end
   end
 
   @doc false
-  def stop(_) do
-    Application.get_env(:logger, :deleted_handlers)
-    |> Enum.each(&:error_logger.add_report_handler/1)
-
-    # We need to do this in another process as the Application
-    # Controller is currently blocked shutting down this app.
-    spawn_link(fn -> Logger.Config.clear_data end)
+  def stop(config) do
+    Logger.Config.deleted_handlers()
+    |> add_handlers()
+    Logger.Config.delete(config)
+  end
 
-    :ok
+  @doc false
+  def config_change(_changed, _new, _removed) do
+    Logger.Config.configure([])
   end
 
   @doc """
   Stops the application without sending messages to error logger.
   """
   def stop() do
-    set = Application.get_env(:logger, :deleted_handlers)
-    Application.put_env(:logger, :deleted_handlers, HashSet.new)
-    _ = Application.stop(:logger)
-    Enum.each(set, &:error_logger.add_report_handler/1)
+    try do
+      Logger.Config.deleted_handlers([])
+    catch
+      :exit, {:noproc, _} ->
+        {:error, {:not_started, :logger}}
+    else
+      deleted_handlers ->
+        result = Application.stop(:logger)
+        add_handlers(deleted_handlers)
+        result
+    end
   end
 
-  defp store_deleted_handlers(list) do
-    Application.put_env(:logger, :deleted_handlers, Enum.into(list, HashSet.new))
+  defp delete_handlers(handlers) do
+    deleted? = fn({handler, delete?}) ->
+        delete? && :error_logger.delete_report_handler(handler) != {:error, :module_not_found}
+      end
+    [] = Enum.filter_map(handlers, deleted?, fn({handler, _}) -> handler end)
+        |> Logger.Config.deleted_handlers()
+    :ok
   end
 
-  defp delete_error_logger_handler(should_delete?, handler, deleted) do
-    if should_delete? and
-        :error_logger.delete_report_handler(handler) != {:error, :module_not_found} do
-      [handler|deleted]
-    else
-      deleted
-    end
+  defp add_handlers(handlers) do
+    Enum.each(handlers, &:error_logger.add_report_handler/1)
   end
 end

lib/logger/lib/logger/config.ex

@@ -4,7 +4,9 @@ defmodule Logger.Config do
   use GenEvent
 
   @name __MODULE__
+  @table __MODULE__
   @data :__data__
+  @deleted_handlers :__deleted_handlers__
 
   def start_link do
     GenServer.start_link(__MODULE__, :ok, name: @name)
@@ -44,22 +46,46 @@ defmodule Logger.Config do
   def translate_backend(other),    do: other
 
   def __data__() do
-    if data = Application.get_env(:logger, @data) do
-      data
+    try do
+      :ets.lookup_element(@table, @data, 2)
+    rescue
+      ArgumentError ->
+        raise "Cannot use Logger, the :logger application is not running"
     else
-      raise "Cannot use Logger, the :logger application is not running"
+      nil ->
+        raise "Cannot use Logger, the :logger application is not running"
+      data ->
+        data
     end
   end
 
-  def clear_data() do
-    Application.delete_env(:logger, @data)
+  def deleted_handlers() do
+    try do
+      :ets.lookup_element(@table, @deleted_handlers, 2)
+    rescue
+      ArgumentError ->
+        []
+    end
+  end
+
+  def deleted_handlers(handlers) do
+    GenEvent.call(Logger, @name, {:deleted_handlers, handlers})
+  end
+
+  def new() do
+    tab = :ets.new(@table, [:named_table, :public, {:read_concurrency, true}])
+    true = :ets.insert_new(@table, [{@data, nil}, {@deleted_handlers, []}])
+    tab
   end
 
+  def delete(@table) do
+    :ets.delete(@table)
+  end
   ## Callbacks
 
   def init(_) do
     # Use previous data if available in case this handler crashed.
-    state = Application.get_env(:logger, @data) || compute_state(:async)
+    state = :ets.lookup_element(@table, @data, 2) || compute_state(:async)
     {:ok, state}
   end
 
@@ -111,6 +137,12 @@ defmodule Logger.Config do
     {:ok, :ok, state}
   end
 
+  def handle_call({:deleted_handlers, new}, state) do
+    old = deleted_handlers()
+    true = :ets.update_element(@table, @deleted_handlers, {2, new})
+    {:ok, old, state}
+  end
+
   ## Helpers
 
   defp compute_mode(state) do
@@ -159,7 +191,7 @@ defmodule Logger.Config do
   end
 
   defp persist(state) do
-    Application.put_env(:logger, @data, state)
+    :ets.update_element(@table, @data, {2, state})
     state
   end
 end

lib/logger/test/logger_test.exs

@@ -234,4 +234,15 @@ defmodule LoggerTest do
     wait_for_handler(Logger, Logger.Config)
     wait_for_handler(:error_logger, Logger.ErrorHandler)
   end
+
+  test "Logger.Config updates config on config_change/3" do
+    :ok = Logger.configure([level: :debug])
+    try do
+      Application.put_env(:logger, :level, :error)
+      assert Logger.App.config_change([level: :error], [], []) === :ok
+      assert Logger.level() === :error
+    after
+      Logger.configure([level: :debug])
+    end
+  end
 end