From 75431d75ff21d0738a6559e4149fde9784bf72d3 Mon Sep 17 00:00:00 2001 From: Aliaksey Artamonau Date: Fri, 19 Nov 2021 14:33:13 +0300 Subject: [PATCH] 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 Reviewed-by: Dave Finlay Well-Formed: Build Bot --- src/leader_registry.erl | 46 +++++++++++++++++++++++----------- test/leader_registry_tests.erl | 45 ++++++++++++++++++++++++++------- 2 files changed, 68 insertions(+), 23 deletions(-) diff --git a/src/leader_registry.erl b/src/leader_registry.erl index cabd296d1e..c3f257ae05 100644 --- a/src/leader_registry.erl +++ b/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 diff --git a/test/leader_registry_tests.erl b/test/leader_registry_tests.erl index 88d40244d0..955556b53d 100644 --- a/test/leader_registry_tests.erl +++ b/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).