Stop Bypass.Instance correctly in dispatch_awaiting_callers/1
#141
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
At the moment `dispatch_awaiting_callers/1` 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.
|
@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 |
We're exactly having this issue (mimiquate/tower_rollbar#36) because of the reason quoted here ☝️ |
|
For what is worth, changes in this PR fixes the flakiness for us. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At the moment it appears that
dispatch_awaiting_callers/1attempts to shut down the current instance usingGenServer.stop(:normal). This causes a crash as the first argument toGenServer.stop/3is meant to be a PID or name, rather than a reason. For example: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/1so 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.