Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition of Tesla.Mock.mock_global/1 #456

Merged

Conversation

sircinek
Copy link
Contributor

@sircinek sircinek commented Mar 18, 2021

Problem:
Intermittent test failures when using Tesla.Mock.mock_global/1

      ** (exit) exited in: GenServer.call(#PID<0.16741.0>, {:update, #Function<2.71048325/1 in Tesla.Mock.agent_set/1>}, 5000)
          ** (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
      code: Tesla.Mock.mock_global(fn
      stacktrace:
        (elixir 1.10.4) lib/gen_server.ex:1023: GenServer.call/3

Current implementation:
agent_set/1 is not atomic, and there's no guarantee pid returned by Process.whereis/1 will be alive when making Agent.update/2 call. That's because Agent process is linked to the test process, but the exit(:shutdown) signal is asynchronous, thus the same process can "leak" between test cases in certain edge cases.

Proposed implementation:
Instead of using plain Agent.start_link/2 use ExUnit.Callbacks.start_supervised!/1, that puts the Tesla.Mock agent under the test supervision tree and guarantees the process is terminated synchronously, thus prevents the leak between test cases.

Agent process is terminated asynchronously and can leak between tests, causing it to fail (update is not atomic, and returned pid might not be alive anymore).
@teamon
Copy link
Member

teamon commented Mar 24, 2021

Thanks!!

@teamon teamon merged commit 4aa32ae into elixir-tesla:master Mar 24, 2021
@ZeLarpMaster
Copy link

ZeLarpMaster commented Apr 12, 2021

Hey there, I was using mock_global in my test_helper.exs because I had to mock a call done by my application on startup (and before tests are executed), but this change broke my tests because start_supervised/2 can only be invoked from the test process and because test_helper.exs isn't a test process. Maybe what I was doing wasn't supported or a good idea, but I was wondering about what's your opinion on that and if you have suggestions on how to solve this issue? Afaik there isn't another mock_global which could be used globally outside tests. Maybe a different mock_global scoped to tests would be better than changing mock_global to be scoped to tests? Not sure on what's your opinion on this.

If you need access, this is the project where I do that and I noticed the issue because of this PR by renovate

@teamon
Copy link
Member

teamon commented Apr 12, 2021

@ZeLarpMaster As a quick answer, you can work around this by replacing:

Tesla.Mock.mock_global(myfun)

with

{:ok, _} = Agent.start_link(myfun, name: Tesla.Mock)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants