Merge branch 'rickard/net-kernel-connect-fix/ERIERL-1241/OTP-19702' into maint-27

Erlang/OTP · Erlang/OTP · commit e01741232f80 · 2025-07-17T09:29:23.000+02:00
* rickard/net-kernel-connect-fix/ERIERL-1241/OTP-19702:
  [kernel] Prevent net_kernel from being blocked on connection setup
diff --git a/lib/kernel/src/net_kernel.erl b/lib/kernel/src/net_kernel.erl
@@ -176,7 +176,7 @@ in the Erlang Reference Manual.
 -define(LISTEN_ID, #listen.listen).
 -define(ACCEPT_ID, #listen.accept).
 
--type connection_state() :: pending | up | up_pending.
+-type connection_state() :: check_pending | pending | up | up_pending.
 -type connection_type() :: normal | hidden.
 
 -include("net_address.hrl").
@@ -1079,7 +1079,7 @@ do_auto_connect_2(Node, ConnId, From, State, ConnLookup) ->
                                            ets:lookup(sys_dist, Node)}),
                     {reply, false, State};
                 _ ->
-                    case setup(Node, ConnId, normal, From, State) of
+                    case setup_check(Node, ConnId, normal, From, State) of
                         {ok, SetupPid} ->
                             Owners = State#state.conn_owners,
                             {noreply,State#state{conn_owners=Owners#{SetupPid => Node}}};
@@ -1094,7 +1094,8 @@ do_auto_connect_2(Node, ConnId, From, State, ConnLookup) ->
 do_explicit_connect([#connection{conn_id = ConnId, state = up}], _, _, ConnId, _From, State) ->
     {reply, true, State};
 do_explicit_connect([#connection{conn_id = ConnId}=Conn], _, _, ConnId, From, State)
-  when Conn#connection.state =:= pending;
+  when Conn#connection.state =:= check_pending;
+       Conn#connection.state =:= pending;
        Conn#connection.state =:= up_pending ->
     Waiting = Conn#connection.waiting,
     ets:insert(sys_dist, Conn#connection{waiting = [From|Waiting]}),
@@ -1103,7 +1104,7 @@ do_explicit_connect([#barred_connection{}], Type, Node, ConnId, From , State) ->
     %% Barred connection only affects auto_connect, ignore it.
     do_explicit_connect([], Type, Node, ConnId, From , State);
 do_explicit_connect(_ConnLookup, Type, Node, ConnId, From , State) ->
-    case setup(Node,ConnId,Type,From,State) of
+    case setup_check(Node,ConnId,Type,From,State) of
         {ok, SetupPid} ->
             Owners = State#state.conn_owners,
             {noreply,State#state{conn_owners=Owners#{SetupPid => Node}}};
@@ -1445,7 +1446,7 @@ handle_info({dist_ctrlr, Ctrlr, Node, SetupPid} = Msg,
 %%
 %% A node has successfully been connected.
 %%
-handle_info({SetupPid, {nodeup,Node,Address,Type,NamedMe} = Nodeup},
+handle_info({SetupPid, {nodeup,Node,Address,Type,NamedMe} = Nodeup} = Msg,
             #state{tick = Tick} = State) ->
     case ets:lookup(sys_dist, Node) of
 	[Conn] when (Conn#connection.state =:= pending)
@@ -1469,7 +1470,8 @@ handle_info({SetupPid, {nodeup,Node,Address,Type,NamedMe} = Nodeup},
             verbose(Nodeup, 1, State1),
             verbose({nodeup,Node,SetupPid,Conn#connection.ctrlr}, 2, State1),
             {noreply, State1};
-	_ ->
+	_Conn ->
+            verbose({bad_request, Msg, _Conn}, 2, State),
 	    SetupPid ! {self(), bad_request},
 	    {noreply, State}
     end;
@@ -1481,9 +1483,14 @@ handle_info({AcceptPid, {accept_pending,MyNode,NodeOrHost,Type}}, State0) ->
     {NameType, Node, Creation,
      ConnLookup, State} = ensure_node_name(NodeOrHost, State0),
     case ConnLookup of
-	[#connection{state=pending}=Conn] ->
+	[#connection{state=Pending}=Conn] when Pending == pending;
+                                               Pending == check_pending ->
 	    if
-		MyNode > Node ->
+                %% If we are in check_pending, we always select the other node.
+                %% We currently have not started the handshake at all, and the
+                %% other node's connection attempt is ongoing, so we select its
+                %% connection.
+		MyNode > Node andalso Pending == pending ->
 		    AcceptPid ! {self(),{accept_pending,nok_pending}},
                     verbose({accept_pending_nok, Node, AcceptPid}, 2, State),
 		    {noreply,State};
@@ -1495,7 +1502,7 @@ handle_info({AcceptPid, {accept_pending,MyNode,NodeOrHost,Type}}, State0) ->
 		    OldOwner = Conn#connection.owner,
                     case maps:is_key(OldOwner, State#state.conn_owners) of
                         true ->
-                            verbose({remark,OldOwner,AcceptPid}, 2, State),
+                            verbose({remark,Node,OldOwner,AcceptPid}, 2, State),
                             ?debug({net_kernel, remark, old, OldOwner, new, AcceptPid}),
                             exit(OldOwner, remarked),
                             receive
@@ -1505,11 +1512,13 @@ handle_info({AcceptPid, {accept_pending,MyNode,NodeOrHost,Type}}, State0) ->
                             end;
                         false ->
                             verbose(
-                              {accept_pending, OldOwner, inconsistency},
+                              {accept_pending, Node, OldOwner, inconsistency},
                               2, State),
                             ok % Owner already exited
                     end,
-		    ets:insert(sys_dist, Conn#connection{owner = AcceptPid}),
+                    ets:insert(sys_dist, Conn#connection{owner = AcceptPid,
+                                                         state = pending,
+                                                         type = Type}),
 		    AcceptPid ! {self(),{accept_pending,ok_pending}},
                     Owners = maps:remove(OldOwner, State#state.conn_owners),
 		    {noreply, State#state{conn_owners=Owners#{AcceptPid => Node}}}
@@ -1642,6 +1651,14 @@ handle_info(transition_period_end,
                                       time = T,
                                       intensity = I}}};
 
+handle_info({setup_check, Node, Pid, Timer, Res} = Msg, State) ->
+    verbose(Msg, 2, State),
+    {noreply, setup(Node, Pid, Timer, Res, State)};
+
+handle_info({setup_check_timeout, Node, Pid} = Msg, State) ->
+    verbose(Msg, 2, State),
+    {noreply, setup_check_timeout(Node, Pid, State)};
+
 handle_info(X, State) ->
     error_msg("Net kernel got ~tw~n",[X]),
     {noreply,State}.
@@ -1829,8 +1846,10 @@ delete_ctrlr(Ctrlr, #state{dist_ctrlrs = DCs} = State) ->
     State#state{dist_ctrlrs = maps:remove(Ctrlr, DCs)}.
 
 nodedown(Conn, Exited, Node, Reason, Type, State) ->
+    verbose({nodedown, Node, Conn#connection.state, Reason}, 2, State),
     case Conn#connection.state of
-	pending ->
+	Pending when Pending == pending;
+                     Pending == check_pending ->
 	    pending_nodedown(Conn, Exited, Node, Type, State);
 	up ->
 	    up_nodedown(Conn, Exited, Node, Reason, Type, State);
@@ -2226,52 +2245,102 @@ spawn_func(_,{From,Tag},M,F,A,Gleader) ->
 %% Set up connection to a new node.
 %% -----------------------------------------------------------
 
-setup(Node, ConnId, Type, From, State) ->
-    case setup_check(Node, State) of
-		{ok, L} ->
-		    Mod = L#listen.module,
-		    LAddr = L#listen.address,
-		    MyNode = State#state.node,
-		    Pid = Mod:setup(Node,
-				    Type,
-				    MyNode,
-				    State#state.type,
-				    State#state.connecttime),
+setup(Node, CheckPid, CheckTimer, CheckRes, State) ->
+    ok = erlang:cancel_timer(CheckTimer, [{async, true}, {info, false}]),
+    unlink(CheckPid),
+    exit(CheckPid, kill),
+    case ets:lookup(sys_dist, Node) of
+        [#connection{owner = CheckPid, state = check_pending} = Conn] ->
+            case CheckRes of
+                {ok, #listen{} = L} ->
+                    Mod = L#listen.module,
+                    LAddr = L#listen.address,
+                    MyNode = State#state.node,
+                    %% We intentionally allow for a full connecttime in
+                    %% Mod:setup() since the check was successful. The
+                    %% setup timer is reset in other places as well...
+                    SetupPid = Mod:setup(Node,
+                                         Conn#connection.type,
+                                         MyNode,
+                                         State#state.type,
+                                         State#state.connecttime),
                     verbose(
-                      {setup,Node,Type,MyNode,State#state.type,Pid},
+                      {setup,Node,Conn#connection.type,
+                       MyNode,State#state.type,SetupPid},
                       2, State),
-		    Addr = LAddr#net_address {
-					      address = undefined,
-					      host = undefined },
-                    Waiting = case From of
-                                  noreply -> [];
-                                  _ -> [From]
-                              end,
-		    ets:insert(sys_dist, #connection{node = Node,
-                                                     conn_id = ConnId,
-						     state = pending,
-						     owner = Pid,
-						     waiting = Waiting,
-						     address = Addr,
-						     type = normal,
-                                                     remote_name_type = static}),
-		    {ok, Pid};
-		Error ->
-		    Error
+                    Addr = LAddr#net_address{address = undefined,
+                                             host = undefined},
+                    ets:insert(sys_dist, Conn#connection{state = pending,
+                                                         owner = SetupPid,
+                                                         address = Addr,
+                                                         type = normal}),
+                    State2 = delete_owner(CheckPid, State),
+                    Owners = State#state.conn_owners,
+                    State2#state{conn_owners = Owners#{SetupPid => Node}};
+                CheckError ->
+                    Failure = {setup_check_failed, Node, CheckError},
+                    verbose(Failure, 2, State),
+                    ?connect_failure(Node, Failure),
+                    pending_nodedown(Conn, CheckPid, Node,
+                                     Conn#connection.type, State)
+            end;
+        _ ->
+            State
+    end.
+
+setup_check_timeout(Node, CheckPid, State) ->
+    case ets:lookup(sys_dist, Node) of
+        [#connection{owner = CheckPid, state = check_pending} = Conn] ->
+            unlink(CheckPid),
+            exit(CheckPid, kill),
+            Failure = {setup_check_failed, Node, setup_check_timeout},
+            verbose(Failure, 2, State),
+            ?connect_failure(Node, Failure),
+            pending_nodedown(Conn, CheckPid, Node,
+                             Conn#connection.type, State);
+        _ ->
+            State
     end.
 
-setup_check(Node, State) ->
+%% Shut up dialyzer warning about no return from SelMod fun. It
+%% should be that way...
+-dialyzer([{nowarn_function, setup_check/5}, no_return]).
+
+setup_check(Node, ConnId, Type, From, State) ->
     Allowed = State#state.allowed,
     case lists:member(Node, Allowed) of
 	false when Allowed =/= [] ->
 	    error_msg("** Connection attempt with "
 		      "disallowed node ~w ** ~n", [Node]),
 	    {error, bad_node};
        _ ->
-            case select_mod(Node, State#state.listen) of
-                {ok, _L}=OK -> OK;
-                Error -> Error
-            end
+            NetKernel = self(),
+            TimeoutTime = State#state.connecttime,
+            Listen = State#state.listen,
+            SelMod = fun () ->
+                             TimeoutMsg = {setup_check_timeout, Node, self()},
+                             Tmr = erlang:send_after(TimeoutTime,
+                                                     NetKernel,
+                                                     TimeoutMsg),
+                             Res = select_mod(Node, Listen),
+                             NetKernel ! {setup_check, Node, self(), Tmr, Res},
+                             %% Wait for net_kernel to kill us...
+                             receive after infinity -> ok end
+                     end,
+            Pid = spawn_link(SelMod),
+            verbose({init_setup_check, Node, Pid, TimeoutTime}, 2, State),
+            Waiting = case From of
+                          noreply -> [];
+                          _ -> [From]
+                      end,
+            ets:insert(sys_dist, #connection{node = Node,
+                                             conn_id = ConnId,
+                                             state = check_pending,
+                                             owner = Pid,
+                                             waiting = Waiting,
+                                             type = Type,
+                                             remote_name_type = static}),
+            {ok, Pid}
     end.
 
 %%
diff --git a/lib/kernel/test/Makefile b/lib/kernel/test/Makefile
@@ -71,6 +71,7 @@ MODULES= \
 	appinc2top \
 	appinc2A \
 	appinc2B \
+	block_select_dist \
 	code_SUITE \
 	code_b_test \
 	code_coverage_SUITE \
diff --git a/lib/kernel/test/block_select_dist.erl b/lib/kernel/test/block_select_dist.erl
@@ -0,0 +1,71 @@
+%%
+%% %CopyrightBegin%
+%%
+%% SPDX-License-Identifier: Apache-2.0
+%%
+%% Copyright Ericsson AB 2025. All Rights Reserved.
+%%
+%% Licensed under the Apache License, Version 2.0 (the "License");
+%% you may not use this file except in compliance with the License.
+%% You may obtain a copy of the License at
+%%
+%%     http://www.apache.org/licenses/LICENSE-2.0
+%%
+%% Unless required by applicable law or agreed to in writing, software
+%% distributed under the License is distributed on an "AS IS" BASIS,
+%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+%% See the License for the specific language governing permissions and
+%% limitations under the License.
+%%
+%% %CopyrightEnd%
+%%
+-module(block_select_dist).
+
+%% A wrapper around gen_tcp_dist with the option to block the select()
+%% call for testing of net_kernel...
+
+-export([block_select/0, unblock_select/0]).
+
+-export([listen/1, accept/1, accept_connection/5,
+	 setup/5, close/1, select/1, is_node_name/1,
+         address/0, setopts/2, getopts/2]).
+
+block_select() ->
+    persistent_term:put(?MODULE, true),
+    ok.
+
+unblock_select() ->
+    persistent_term:put(?MODULE, false),
+    ok.
+
+select(A1) ->
+    WaitOnBlock = fun WaitOnBlock () ->
+                          case persistent_term:get(?MODULE, false) of
+                              true ->
+                                  receive after 1000 -> ok end,
+                                  WaitOnBlock();
+                              _ ->
+                                  ok
+                          end
+                  end,
+    WaitOnBlock(),
+    gen_tcp_dist:select(A1).
+
+listen(A1) ->
+    gen_tcp_dist:listen(A1).
+accept(A1) ->
+    gen_tcp_dist:accept(A1).
+accept_connection(A1, A2, A3, A4, A5) ->
+    gen_tcp_dist:accept_connection(A1, A2, A3, A4, A5).
+setup(A1, A2, A3, A4, A5) ->
+    gen_tcp_dist:setup(A1, A2, A3, A4, A5).
+close(A1) ->
+    gen_tcp_dist:close(A1).
+is_node_name(A1) ->
+    gen_tcp_dist:is_node_name(A1).
+address() ->
+    gen_tcp_dist:address().
+setopts(A1, A2) ->
+    gen_tcp_dist:setopts(A1, A2).
+getopts(A1, A2) ->
+    gen_tcp_dist:getopts(A1, A2).
diff --git a/lib/kernel/test/erl_distribution_SUITE.erl b/lib/kernel/test/erl_distribution_SUITE.erl
