Repository: elixir-protobuf/protobuf (elixir)

Issue:
Application start is not idempotent
As @igormq reported in gRPC repository [here](https://github.com/elixir-grpc/grpc/issues/165) (but it is actually a problem with this library), starting `:protobuf` is not idempotent, which means that if you need to restart the application (which can happen on purpose on some environments, such as how Nerve's Shoehorn works) protobuf refuses to start again.

This is due to this code here: https://github.com/tony612/protobuf-elixir/blob/master/lib/protobuf/extension.ex#L134-L136

I'm not sure why that is there, but I think we need to rethink/remove that.

In order to reproduce the problem, just do:

```elixir
Application.ensure_started(:protobuf)
Application.stop(:protobuf)
Application.ensure_started(:protobuf)
```

The expected behavior would be to protobuf app to just start, but since it is using persistent_term to store and it is crashing if it is set, then it cause the application to throw and not start, requiring a full VM restart (or unsetting the persistent_term by hand, which seems like a hack messing with protobuf internals).

I can contribute with a fix, but I first need to understand why that is there and discuss approaches to solve that.

Thanks

Pull request:
Ensure proper cleanup on application shutdown
This will leave the BEAM in a clean state once the app shuts down by dropping all extensions currently loaded. As a consequence, this also ensures clean restarts. It closes #145, as an alternative approach to fix #130.

Since we dropped support to OTP 21, I also seized the opportunity to remove the `GlobalStore` wrapper around `:persistent_term`.

Patch:
diff --git a/lib/application.ex b/lib/application.ex
index 6b7bf66..624b3f4 100644
--- a/lib/application.ex
+++ b/lib/application.ex
@@ -9,20 +9,27 @@ defmodule Protobuf.Application do
   """
 
   @doc false
+  @impl true
   def start(_type, _args) do
     if Application.get_env(:protobuf, :extensions, :disabled) == :enabled do
       mods = get_all_modules()
-      Protobuf.Extension.cal_extensions(mods)
+      Protobuf.Extension.__cal_extensions__(mods)
     else
       # Extensions in Protobuf should always be calculated for generating code
       {:ok, mods} = :application.get_key(:protobuf, :modules)
-      Protobuf.Extension.cal_extensions(mods)
+      Protobuf.Extension.__cal_extensions__(mods)
     end
 
     children = []
     Supervisor.start_link(children, strategy: :one_for_one)
   end
 
+  @impl true
+  def stop(_state) do
+    Protobuf.Extension.__unload_extensions__()
+    :ok
+  end
+
   defp get_all_modules() do
     case :code.get_mode() do
       :embedded ->
diff --git a/lib/protobuf/extension.ex b/lib/protobuf/extension.ex
index 3cc8b34..450fe03 100644
--- a/lib/protobuf/extension.ex
+++ b/lib/protobuf/extension.ex
@@ -4,8 +4,7 @@ defmodule Protobuf.Extension do
   let you set extra fields for previously defined messages(even for messages in other packages)
   without changing the original message.
 
-  **This is an experimental feature**, to use it, Erlang should be >= 21.2 because `:persistent_term`
-  is used and config should be set:
+  **This is an experimental feature**, the following config should be set to use it:
 
       # Without this, modules won't be scanned to get extensions metadata.
       # Functions like `get_extension` and `put_extension` still exist, but they don't work.
@@ -38,7 +37,6 @@ defmodule Protobuf.Extension do
       Foo.put_extension(foo, Ext.PbExtension, :my_custom, "Custom field")
       Foo.get_extension(foo, Ext.PbExtension, :my_custom)
   """
-  alias Protobuf.GlobalStore
 
   @doc "The actual function for `put_extension`"
   @spec put(module, map, module, atom, any) :: map
@@ -97,7 +95,7 @@ defmodule Protobuf.Extension do
 
   @doc false
   def get_extension_props_by_tag(extendee, tag) do
-    case GlobalStore.get({Protobuf.Extension, extendee, tag}, nil) do
+    case :persistent_term.get({Protobuf.Extension, extendee, tag}, nil) do
       nil ->
         nil
 
@@ -115,7 +113,7 @@ defmodule Protobuf.Extension do
   end
 
   @doc false
-  def cal_extensions(mods) do
+  def __cal_extensions__(mods) do
     mods
     |> Enum.filter(fn mod ->
       if to_string(mod) =~ ~r/\.PbExtension$/ && Code.ensure_loaded?(mod) do
@@ -131,12 +129,19 @@ defmodule Protobuf.Extension do
         fnum = ext.field_props.fnum
         fnum_key = {Protobuf.Extension, ext.extendee, fnum}
 
-        if GlobalStore.get(fnum_key, nil) do
+        if :persistent_term.get(fnum_key, nil) do
           raise "Extension #{inspect(ext.extendee)}##{fnum} already exists"
         end
 
-        GlobalStore.put(fnum_key, mod)
+        :persistent_term.put(fnum_key, mod)
       end)
     end)
   end
+
+  @doc false
+  def __unload_extensions__ do
+    for {{Protobuf.Extension, _extendee, _tag} = key, _mod} <- :persistent_term.get() do
+      :persistent_term.erase(key)
+    end
+  end
 end
diff --git a/lib/protobuf/global_store.ex b/lib/protobuf/global_store.ex
deleted file mode 100644
index 728fda8..0000000
--- a/lib/protobuf/global_store.ex
+++ /dev/null
@@ -1,23 +0,0 @@
-defmodule Protobuf.GlobalStore do
-  @moduledoc false
-
-  if function_exported?(:persistent_term, :get, 2) &&
-       function_exported?(:persistent_term, :put, 2) do
-    def put(key, value) do
-      :persistent_term.put(key, value)
-    end
-
-    def get(key, default) do
-      :persistent_term.get(key, default)
-    end
-  else
-    # Without :persistent_term, this is not supported
-    def put(key, value) do
-      :ok
-    end
-
-    def get(_key, default) do
-      default
-    end
-  end
-end


Test patch:
diff --git a/test/protobuf/application_test.exs b/test/protobuf/application_test.exs
new file mode 100644
index 0000000..a3f5667
--- /dev/null
+++ b/test/protobuf/application_test.exs
@@ -0,0 +1,17 @@
+defmodule Protobuf.ApplicationTest do
+  use ExUnit.Case, async: false
+
+  test "unloads all extensions on shutdown and reloads upon start" do
+    assert loaded_extensions() > 0
+
+    assert :ok = Application.stop(:protobuf)
+    assert loaded_extensions() == 0
+
+    assert :ok = Application.start(:protobuf)
+    assert loaded_extensions() > 0
+  end
+
+  defp loaded_extensions do
+    Enum.count(:persistent_term.get(), &match?({{Protobuf.Extension, _, _}, _}, &1))
+  end
+end
diff --git a/test/protobuf/extension_test.exs b/test/protobuf/extension_test.exs
index ec1c252..606c404 100644
--- a/test/protobuf/extension_test.exs
+++ b/test/protobuf/extension_test.exs
@@ -1,15 +1,13 @@
 defmodule Protobuf.ExtensionTest do
   use ExUnit.Case, async: true
 
-  alias Protobuf.GlobalStore
-
   alias TestMsg.Ext
 
   test "extension persistent works" do
-    assert Ext.PbExtension = GlobalStore.get({Protobuf.Extension, Ext.Foo1, 1047}, nil)
-    assert Ext.PbExtension = GlobalStore.get({Protobuf.Extension, Ext.Foo1, 1048}, nil)
-    assert Ext.PbExtension = GlobalStore.get({Protobuf.Extension, Ext.Foo1, 1049}, nil)
-    assert Ext.PbExtension = GlobalStore.get({Protobuf.Extension, Ext.Foo2, 1047}, nil)
+    assert Ext.PbExtension == :persistent_term.get({Protobuf.Extension, Ext.Foo1, 1047})
+    assert Ext.PbExtension == :persistent_term.get({Protobuf.Extension, Ext.Foo1, 1048})
+    assert Ext.PbExtension == :persistent_term.get({Protobuf.Extension, Ext.Foo1, 1049})
+    assert Ext.PbExtension == :persistent_term.get({Protobuf.Extension, Ext.Foo2, 1047})
   end
 
   test "extension get/put work" do