Browse Source

Do not kill other pid in custom conflict resolution.

Roberto Ostinelli 3 years ago
parent
commit
934f376ef5
3 changed files with 19 additions and 12 deletions
  1. 7 6
      src/syn_event_handler.erl
  2. 10 4
      src/syn_registry.erl
  3. 2 2
      test/syn_registry_SUITE.erl

+ 7 - 6
src/syn_event_handler.erl

@@ -56,7 +56,8 @@
 %% into the callback for both of the conflicting processes.
 %%
 %% If implemented, this method MUST return the `pid()' of the process that you wish to keep. The other process will not
-%% be killed, so you will have to decide what to do with it.
+%% be killed, so you will have to decide what to do with it. If the custom conflict resolution method does not return
+%% one of the two Pids, or if the method crashes, none of the Pids will be killed and the conflicting name will be freed.
 %%
 %% Important Note: the conflict resolution method will be called on the two nodes where the conflicting processes are running on.
 %% Therefore, this method MUST be defined in the same way across all nodes of the cluster and have the same effect
@@ -213,21 +214,21 @@ call_event_handler(CallbackMethod, Args) ->
     Name :: any(),
     {Pid1 :: pid(), Meta1 :: any(), Time1 :: non_neg_integer()},
     {Pid2 :: pid(), Meta2 :: any(), Time2 :: non_neg_integer()}
-) -> PidToKeep :: pid() | undefined.
+) -> {PidToKeep :: pid() | undefined, KillOtherPid :: boolean()}.
 do_resolve_registry_conflict(Scope, Name, {Pid1, Meta1, Time1}, {Pid2, Meta2, Time2}) ->
     CustomEventHandler = get_custom_event_handler(),
     case erlang:function_exported(CustomEventHandler, resolve_registry_conflict, 4) of
         true ->
             try CustomEventHandler:resolve_registry_conflict(Scope, Name, {Pid1, Meta1, Time1}, {Pid2, Meta2, Time2}) of
-                PidToKeep when is_pid(PidToKeep) -> PidToKeep;
-                _ -> undefined
+                PidToKeep when is_pid(PidToKeep) -> {PidToKeep, false};
+                _ -> {undefined, false}
 
             catch Class:Reason ->
                 error_logger:error_msg(
                     "SYN[~s] Error ~p in custom handler resolve_registry_conflict: ~p",
                     [?MODULE, Class, Reason]
                 ),
-                undefined
+                {undefined, false}
             end;
 
         _ ->
@@ -238,7 +239,7 @@ do_resolve_registry_conflict(Scope, Name, {Pid1, Meta1, Time1}, {Pid2, Meta2, Ti
                 true -> Pid1;
                 _ -> Pid2
             end,
-            PidToKeep
+            {PidToKeep, true}
     end.
 
 %% ===================================================================

+ 10 - 4
src/syn_registry.erl

@@ -571,7 +571,7 @@ resolve_conflict(Scope, Name, {Pid, Meta, Time}, {TablePid, TableMeta, TableTime
     table_by_pid = TableByPid
 } = State) ->
     %% call conflict resolution
-    PidToKeep = syn_event_handler:do_resolve_registry_conflict(
+    {PidToKeep, KillOtherPid} = syn_event_handler:do_resolve_registry_conflict(
         Scope,
         Name,
         {Pid, Meta, Time},
@@ -587,13 +587,16 @@ resolve_conflict(Scope, Name, {Pid, Meta, Time}, {TablePid, TableMeta, TableTime
             %% update locally, the incoming sync_register will update with the time coming from remote node
             update_local_table(Name, TablePid, {Pid, Meta, Time, undefined}, TableByName, TableByPid),
             %% kill
-            exit(TablePid, {syn_resolve_kill, Name, TableMeta}),
+            case KillOtherPid of
+                true -> exit(TablePid, {syn_resolve_kill, Name, TableMeta});
+                false -> ok
+            end,
             %% callbacks
             syn_event_handler:call_event_handler(on_process_unregistered, [Scope, Name, TablePid, TableMeta]),
             syn_event_handler:call_event_handler(on_process_registered, [Scope, Name, Pid, Meta]);
 
         TablePid ->
-            %% -> we keep the local pid
+            %% -> we keep the local pid, remote pid will be killed by the other node in the conflict
             error_logger:info_msg("SYN[~s<~s>] Registry CONFLICT for name ~p: ~p vs ~p -> keeping local: ~p",
                 [?MODULE, Scope, Name, Pid, TablePid, TablePid]
             ),
@@ -611,7 +614,10 @@ resolve_conflict(Scope, Name, {Pid, Meta, Time}, {TablePid, TableMeta, TableTime
             maybe_demonitor(TablePid, TableByPid),
             remove_from_local_table(Name, TablePid, TableByName, TableByPid),
             %% kill local, remote will be killed by other node performing the same resolve
-            exit(TablePid, {syn_resolve_kill, Name, TableMeta}),
+            case KillOtherPid of
+                true -> exit(TablePid, {syn_resolve_kill, Name, TableMeta});
+                false -> ok
+            end,
             %% callback
             syn_event_handler:call_event_handler(on_process_unregistered, [Scope, Name, TablePid, TableMeta])
     end.

+ 2 - 2
test/syn_registry_SUITE.erl

@@ -1492,13 +1492,13 @@ three_nodes_custom_event_handler_conflict_resolution(Config) ->
     1 = rpc:call(SlaveNode2, syn, registry_count, [custom_scope_bc, SlaveNode1]),
     0 = rpc:call(SlaveNode2, syn, registry_count, [custom_scope_bc, SlaveNode2]),
 
-    %% process alive
+    %% process alive (discarded process does not get killed with a custom handler)
     syn_test_suite_helper:assert_wait(
         true,
         fun() -> rpc:call(SlaveNode1, erlang, is_process_alive, [PidOn1]) end
     ),
     syn_test_suite_helper:assert_wait(
-        false,
+        true,
         fun() -> rpc:call(SlaveNode2, erlang, is_process_alive, [PidOn2]) end
     ).