Implement leader_registry:unregister_name/1.

When gen_server (or similar behaviors) terminates due to an error in
the init function, it unregisters the name. Since leader_registry
didn't implement unregistration, this would result in the original
error being hidden by exit(not_supported) produced by
leader_registry:unregister_name/1.

Change-Id: I40b09177c7a08577e663f4f0217c3e63f8a3db72
Reviewed-on: https://review.couchbase.org/c/ns_server/+/166067
Tested-by: Aliaksey Artamonau <aliaksey.artamonau@couchbase.com>
Reviewed-by: Dave Finlay <dave.finlay@couchbase.com>
Well-Formed: Build Bot <build@couchbase.com>

src/leader_registry.erl

@@ -15,7 +15,8 @@
 %%
 %%  - processes are only registered on a master node
 %%  - processes live long
-%%  - there's no need to unregister processes
+%%  - names are unregistered only after the corresponding process has or about
+%%    to terminate
 %%  - it's uncommon to look for a name that is not registered
 %%
 %% Brief summary of how things work.
@@ -31,9 +32,8 @@
 %%  monitored and removed from the cache if the process itself or the link to
 %%  the master node dies.
 %%
-%%  - Since processes cannot be unregistered, there's no need to do anything
-%%  special about it. Cache invalidation relies on the regular Erlang
-%%  monitors.
+%%  - Since names are unregistered when the corresponding process dies, cache
+%%  invalidation relies on regular process monitors.
 
 -module(leader_registry).
 
@@ -66,12 +66,14 @@ start_link() ->
 register_name(Name, Pid) ->
     call({if_leader, {register_name, Name, Pid}}).
 
+%% It's assumed that unregister_name/1 is only called by the registered
+%% process itself when it's about to terminate.
 unregister_name(Name) ->
     call({if_leader, {unregister_name, Name}}).
 
 whereis_name(Name) ->
     case get_cached_name(Name) of
-        {ok, Pid} ->
+        {ok, Pid, _MRef} ->
             Pid;
         not_found ->
             call({whereis_name, Name});
@@ -165,7 +167,7 @@ handle_leader_call({unregister_name, Name}, From, State) ->
 
 handle_register_name(Name, Pid, From, State) ->
     case get_cached_name(Name) of
-        {ok, OtherPid} ->
+        {ok, OtherPid, _MRef} ->
             reply_error(From, {duplicate_name, Name, Pid, OtherPid}),
             State;
         not_found ->
@@ -174,13 +176,28 @@ handle_register_name(Name, Pid, From, State) ->
             State
     end.
 
-handle_unregister_name(_Name, From, State) ->
-    reply_error(From, not_supported),
+handle_unregister_name(Name, From, State) ->
+    {CallerPid, _} = From,
+    case get_cached_name(Name) of
+        {ok, Pid, MRef} ->
+            case Pid =:= CallerPid of
+                true ->
+                    ?log_info("Process ~p unregistered as '~p'", [Pid, Name]),
+                    erlang:demonitor(MRef, [flush]),
+                    invalidate_name(Name, Pid),
+                    reply(From, ok);
+                false ->
+                    reply_error(From, not_supported)
+            end;
+        not_found ->
+            reply(From, ok)
+    end,
+
     State.
 
 handle_whereis_name(Name, From, #state{leader = Leader} = State) ->
     case get_cached_name(Name) of
-        {ok, Pid} ->
+        {ok, Pid, _MRef} ->
             reply(From, Pid),
             State;
         not_found ->
@@ -259,13 +276,13 @@ maybe_cache_name(Name, Pid) when is_pid(Pid) ->
     case get_cached_name(Name) of
         not_found ->
             cache_name(Name, Pid);
-        {ok, CachedPid} ->
+        {ok, CachedPid, _MRef} ->
             true = (CachedPid =:= Pid)
     end.
 
 cache_name(Name, Pid) ->
-    _ = erlang:monitor(process, Pid),
-    true = ets:insert_new(?TABLE, [{{name, Name}, Pid},
+    MRef = erlang:monitor(process, Pid),
+    true = ets:insert_new(?TABLE, [{{name, Name}, Pid, MRef},
                                    {{pid, Pid}, Name}]),
     ok.
 
@@ -282,8 +299,9 @@ get_cached_name(Name) ->
     try ets:lookup(?TABLE, {name, Name}) of
         [] ->
             not_found;
-        [{_, Pid}] when is_pid(Pid) ->
-            {ok, Pid}
+        [{_, Pid, MRef}] when is_pid(Pid),
+                              is_reference(MRef) ->
+            {ok, Pid, MRef}
     catch
         error:badarg ->
             not_running

test/leader_registry_tests.erl

@@ -28,23 +28,34 @@ basic_test__() ->
     Wait = fun R() -> receive Msg -> Parent ! {self(), Msg}, R() end end,
 
     A = spawn_link(Wait),
-    B = spawn_link(Wait),
+    B = spawn_link(fun R() ->
+                           receive
+                               {unregister, Name} ->
+                                   leader_registry:unregister_name(Name),
+                                   Parent ! {self(), unregistered},
+                                   R();
+                               _ ->
+                                   R()
+                           end
+                   end),
+    C = spawn_link(Wait),
 
     undefined = leader_registry:whereis_name(a),
     undefined = leader_registry:whereis_name(b),
 
     ?assertExit(not_a_leader, leader_registry:register_name(a, A)),
+    ?assertExit(not_a_leader, leader_registry:unregister_name(a)),
 
     gen_event:sync_notify(leader_events, {new_leader, node()}),
 
-    yes = leader_registry:register_name(a, A),
-    A = leader_registry:whereis_name(a),
+    lists:foreach(
+      fun ({Name, Pid}) ->
+              yes = leader_registry:register_name(Name, Pid),
+              Pid = leader_registry:whereis_name(Name)
+      end, [{a, A}, {b, B}, {c, C}]),
 
     ?assertExit({duplicate_name, _, _, _}, leader_registry:register_name(a, B)),
 
-    yes = leader_registry:register_name(b, B),
-    B = leader_registry:whereis_name(b),
-
     leader_registry:send(a, test),
     receive
         {A, test} ->
@@ -54,17 +65,33 @@ basic_test__() ->
             exit(no_message_received)
     end,
 
-    ?assertExit({badarg, _}, leader_registry:send(c, test)),
+    ?assertExit({badarg, _}, leader_registry:send(d, test)),
 
     misc:unlink_terminate_and_wait(A, shutdown),
     wait_not_registered(a),
 
+    ?assertExit(not_supported, leader_registry:unregister_name(b)),
+
+    leader_registry:send(b, {unregister, b}),
+    receive
+        {B, unregistered} ->
+            ok
+    after
+        1000 ->
+            exit(unregister_timeout)
+    end,
+    undefined = leader_registry:whereis_name(b),
+
+    %% Unknown names should work.
+    ok = leader_registry:unregister_name(b),
+
     gen_event:sync_notify(leader_events, {new_leader, undefined}),
     %% make sure leader_registry has processed the notification
     _ = sys:get_state(leader_registry),
-    undefined = leader_registry:whereis_name(b),
+    undefined = leader_registry:whereis_name(c),
 
-    misc:unlink_terminate_and_wait(B, shutdown).
+    misc:unlink_terminate_and_wait(B, shutdown),
+    misc:unlink_terminate_and_wait(C, shutdown).
 
 wait_not_registered(Name) ->
     wait_not_registered(Name, 1000).