PSPDFKit-labs / bypass

Bypass provides a quick way to create a custom plug that can be put in place instead of an actual HTTP server to return prebaked responses to client requests.
https://hex.pm/packages/bypass
MIT License
951 stars 107 forks source link

Stop `Bypass.Instance` correctly in `dispatch_awaiting_callers/1` #141

Open c-brenn opened 4 months ago

c-brenn commented 4 months ago

At the moment it appears that dispatch_awaiting_callers/1 attempts to shut down the current instance using GenServer.stop(:normal). This causes a crash as the first argument to GenServer.stop/3 is meant to be a PID or name, rather than a reason. For example:

** (stop) exited in: GenServer.stop(:normal, :normal, :infinity)
    ** (EXIT) no process: the process is not alive or there's no process currently associated with the given name, possibly because its application isn't started
    (elixir 1.16.0) lib/gen_server.ex:1061: GenServer.stop/3
    (bypass 2.1.0) lib/bypass/instance.ex:385: Bypass.Instance.dispatch_awaiting_callers/1
    (bypass 2.1.0) lib/bypass/instance.ex:62: Bypass.Instance.handle_info/2
    (stdlib 5.2) gen_server.erl:1095: :gen_server.try_handle_info/3
    (stdlib 5.2) gen_server.erl:1183: :gen_server.handle_msg/6
    (stdlib 5.2) proc_lib.erl:241: :proc_lib.init_p_do_apply/3
Last message: {:DOWN, #Reference<0.1124410943.1226571777.107399>, :process, #PID<0.6661.0>, :shutdown}

This appears to happen when the ExUnit test that spawned the bypass instance has exited, but the bypass instance is still awaiting a request. For example if the test spawned an async process that will make a request to a bypass stub.

This commit refactors dispatch_awaiting_callers/1 so that it now returns either {:noreply, state} or {:stop, ...} so that it can handle any awaiting callers and then shut down the GenServer which seems to be the intended behaviour of the current code.

c-brenn commented 2 months ago

@bszaf apologies for the ping, saw you had recently been reviewing PRs so you seemed like the best person to ask 😇

At work we've run into this bug a few times now in our codebases and are wondering if you would consider incorporating this change