Why does gen_server:reply/2 work in some instances while causing timeouts in others

Question

I have problems getting gen_server:reply to work in some but not all cases in my code although the code seems to me to be similar in structure from the areas it works and it doesn't. And I don't know if this is due to some conceptual misunderstanding or incompleteness of the gen_server:reply/.

I have created MRE code as seen below (with EUnit tests and all ready to plug and play) I experience that the test function setup_test() succeeds whereas the function setup_test_move_rs_both_receive() doesn't. The latter creates a 'hanging'/time out for the mve_rps_game:move/2 function.

Why is that, and how can I deal with it?

-module(mve_rps_game).
-behaviour(gen_server).
-include_lib("eunit/include/eunit.hrl").

-export([init/1, handle_cast/2, handle_call/3, start/0, setup_game/2, move/2, test_all/0]). 

%------------------------Internal functions to game coordinator module

make_move(Choice, OtherRef, PlayersMap, CurMoveKey, OtherMoveKey) ->
    case maps:get(OtherMoveKey, PlayersMap, badkey) of
        badkey ->            
            PlayersMap2 = maps:put(CurMoveKey, Choice, PlayersMap),
            {noreply, PlayersMap2};
        OtherChoice ->
            io:format(user, "~n Choice, OtherChoice, CurPid, OtherRef: ~w ~w ~w ~w~n",[Choice, OtherChoice, self(), OtherRef]),
            gen_server:reply(OtherRef, Choice),
            {reply, OtherChoice, PlayersMap}           
    end.
%-----------------Init game-coordinator and its handle_call functions

init(_Args) ->    
    PlayersMap = #{}, 
    {ok,PlayersMap}.

handle_call({move, Choice}, From, PlayersMap = #{start:= {P1Ref, P2Ref}}) ->    
    {P1id, _} = P1Ref,
    {P2id, _} = P2Ref,
    {CurId, _} = From,
    case CurId of
        P1id ->            
            make_move(Choice, P2Ref, PlayersMap, p1_move, p2_move);
        P2id ->
            make_move(Choice, P1Ref, PlayersMap, p2_move, p1_move);
        _Other ->
            {reply, {error, not_a_player}, PlayersMap}
    end;

handle_call({set_up, Name}, From, PlayersMap) ->
    case maps:is_key(prev_player, PlayersMap) of
        %Adds req number of rounds as a key containing player name and ref for postponed reply
        false ->           
            PlayersMap2 = maps:put(prev_player,{Name, From}, PlayersMap),
            {noreply, PlayersMap2};
        
        %Sends message back to previous caller and current caller to setup game
        true -> 
            case maps:get(prev_player, PlayersMap, badkey) of                
                {OtherPlayer, OtherRef} ->  
                    gen_server:reply(OtherRef, {ok, Name}),                    
                    PlayersMap2 = maps:remove(prev_player, PlayersMap),                    
                    %Make key start to indicate that game is going on, and with References to two players. 
                    PlayersMap3 = PlayersMap2#{start => {From, OtherRef}}, 
                    {reply, {ok, OtherPlayer}, PlayersMap3};
                _ -> 
                    {reply, error, PlayersMap}
            end        
    end.

handle_cast(_Msg, State) ->
    {noreply, State}.

%------------- CALLS to Rps game -------------------
start() ->
    {ok, Pid} = gen_server:start(?MODULE,[], []),
    {ok, Pid}.

setup_game(Coordinator, Name) -> 
    gen_server:call(Coordinator, {set_up, Name}, infinity).

move(Coordinator, Choice) ->     
    gen_server:call(Coordinator, {move, Choice}, infinity).

%--------------- EUnit Test section ------------

setup_test() ->
    {"Queue up for a game",
     fun() ->
             {ok, CoordinatorPid} = mve_rps_game:start(),
             Caller = self(),
             spawn(fun() -> State = mve_rps_game:setup_game(CoordinatorPid, "player1"),
                            Caller ! State end),
             timer:sleep(1000),             
             {ok, OtherPlayer} = mve_rps_game:setup_game(CoordinatorPid, "player2"), 
             ProcessState = 
                receive             
                    State -> State
                end,
            ?assertMatch(ProcessState, {ok, "player2"}),
            ?assertMatch({ok, OtherPlayer}, {ok, "player1"})            
     end}.

queue_up_test_move_rs_both_receive() ->
    {"Testing that both players recieve answer in rock to scissor",
    fun() ->
        {ok, CoordinatorPid} = mve_rps_game:start(),
        Caller = self(),
        spawn(fun() ->
            {ok, _OtherPlayer} = mve_rps_game:setup_game(CoordinatorPid, "player1"),
            State = mve_rps_game:move(CoordinatorPid, rock),
            Caller ! State
        end),
        timer:sleep(1000),
        {ok, _OtherPlayer} = mve_rps_game:setup_game(CoordinatorPid, "player2"),
        Result2 = mve_rps_game:move(CoordinatorPid, scissor),
        io:format(user, "Result2: ~w~n",[Result2]),    
        ?assertMatch(Result2, rock),
        ProcessState = receive
            State -> State
        end,
        ?assertMatch(ProcessState, win)    
        end}.

test_all() ->
    eunit:test(
      [
        setup_test()
        ,queue_up_test_move_rs_both_receive()
      ], [verbose]).

Answer 1

I didn't find a way to solve your issue but I can point you to the place in the code where things are going astray…

When you call the function make_move/5 from the first clause of handle_call/3, you're passing is as the last parameters either p1_move, p2_move or p2_move, p1_move, but those keys are never added to the PlayersMap so maps:get/3 will always return badkey.

In that case you're not replying to the client (i.e. you return {noreply, PlayersMap2} and you don't keep the From from handle_call/3 anywhere… So, in essence, you never respond to any call to mve_rps_game:move/2. That function uses infinity as its time-out value for gen_server:call/3, so… eventually the tests themselves timeout.

I'm not entirely sure what the fix should be, but you should probably start by checking how/when to respond to your client in make_move/5 (or in the first clause of handle_call/3).

Edit: Found the Issue

(For an explanation of this change, check the comments below)

The way to solve the issue is to keep track of the last message received by each user and not just the first one. Making the following changes you'll be able to move past the timeout issue. Your tests will still fail, but for a different reason…

    case CurId of
        P1id ->            
            make_move(Choice, P2Ref, PlayersMap#{start := {From, P2Ref}}, p1_move, p2_move);
        P2id ->
            make_move(Choice, P1Ref, PlayersMap#{start := {P1Ref, From}}, p2_move, p1_move);
        _Other ->
            {reply, {error, not_a_player}, PlayersMap}
    end;

The main chang is the adjustment of start within PlayersMap. As a matter of fact, that item should be called last_message or something along those lines, but I would leave that up to you.

Answer 2

Ok, I finely figured out, and I was not even that far off in my other posts regarding the use and concept of make_ref() seen here. So the problem is based on a conceptual incompleteness of the pid() vs. reference() and From within gen_server.

The reason why the code causes a timeout is because the gen_server:reply(OtherRef, Choice) uses a reference that was saved in the gen_servers state in a former gen_server:call/3. Therefore it is trying to reply to a call that already was answered, whereas the new call is not, because the reference/tag of the new call isn't stored.