Add test for race condition in community gardens

[cshintov]

If the register function uses get and update instead of get_and_update concurrent operations results in non-unique ids.

[github-actions]

Thank you for contributing to exercism/elixir 💜 🎉. This is an automated PR comment 🤖 for the maintainers of this repository that helps with the PR review process. You can safely ignore it and wait for a maintainer to review your changes.

Based on the files changed in this PR, it would be good to pay attention to the following details when reviewing the PR:

• General steps

  • 🏆 Does this PR need to receive a label with a reputation modifier (x:size/{tiny,small,medium,large,massive})? (A medium reputation amount is awarded by default, see docs)

• Any exercise changed

  • 👤 Does the author of the PR need to be added as an author or contributor in <exercise>/.meta/config.json (see docs)?
  • 🔬 Do the analyzer and the analyzer comments exist for this exercise? Do they need to be changed?
  • 📜 Does the design file (<exercise>/.meta/design.md) need to be updated to document new implementation decisions?

• Concept exercise changed

  • 🌲 Do prerequisites and practices in config.json need to be updated?
  • 📖 Does the concept introduction provide all necessary information to solve this exercise?

• Concept exercise tests changed

  • ⚪️ Are all tests un-skipped?
  • 🔢 Are all tests annotated with @tag task_id?
  • 🐈 Can all tests be understood by reading the test's block only (e.g. no module attributes, no setup functions)?

---

Automated comment created by PR Commenter 🤖.

[angelikatyborska]

Hi! Thank you for bringing this problem to my attention 🙏

I am unable to write a solution that would fail the new test on my machine. I tried this:

  def register(pid, register_to) do
    state = Agent.get(pid, fn state -> state end)
    %{registry: registry, next_id: next_id} = state
    new_plot = %Plot{plot_id: next_id, registered_to: register_to}
    updated = Map.put(registry, next_id, new_plot)
    state = %{state | registry: updated, next_id: next_id + 1}
    Agent.update(pid, fn x -> state end)

    new_plot
  end

I suspect that due to the nature of the problem (concurrency and being unlucky with timing), it might be impossible to write a test that would fail reliably if Agent.get_and_update was not used.

Maybe it would be a better idea to add an analyzer check that ensures Agent.get_and_update is called from CommunityGarden.register?

Some links:

• This is the analyzer repository https://github.com/exercism/elixir-analyzer
• This is an example of a similar check for a different exercise: https://github.com/exercism/elixir-analyzer/blob/198d0da32b9efeaf14c2b132e1e14b0523d38aae/lib/elixir_analyzer/test_suite/newsletter.ex#L112-L117
• And this is where the text for the comments is stored: https://github.com/exercism/website-copy/tree/main/analyzer-comments/elixir (different repository)

[angelikatyborska]

Task nr 6 does not exist. I believe you wanted to add this test to task 3 instead.

[angelikatyborska]

In learning exercises, we try to avoid using not-yet-learned concepts anywhere in the exercise, including tests. At this point in the syllabus, tasks weren't learned yet. Could this test be rewritten to use only concurrency basics like spawn, send, and receive?

[cshintov]

Hi! Thank you for bringing this problem to my attention 🙏

I am unable to write a solution that would fail the new test on my machine. I tried this:

  def register(pid, register_to) do
    state = Agent.get(pid, fn state -> state end)
    %{registry: registry, next_id: next_id} = state
    new_plot = %Plot{plot_id: next_id, registered_to: register_to}
    updated = Map.put(registry, next_id, new_plot)
    state = %{state | registry: updated, next_id: next_id + 1}
    Agent.update(pid, fn x -> state end)

    new_plot
  end

I suspect that due to the nature of the problem (concurrency and being unlucky with timing), it might be impossible to write a test that would fail reliably if Agent.get_and_update was not used.

Actually I misunderstood the cause of the problem.

I tried your solution and found no problem with get and update as you said.

So I tried my solution again and it was giving me non-unique ids. The problem was not get and update, it was how I used them.

  def register(pid, register_to) do
    new_id = Agent.get(pid, & &1.id) + 1

    new_plot = %Plot{plot_id: new_id, registered_to: register_to}
    Agent.update(pid, &%{id: new_id, plots: [new_plot | &1.plots]})

    new_plot
  end

I first extract the current latest id, construct new plot with incremented id and then finally makes the update. This causes concurrent register to give non-unique ids.

Gives error!

No errors!

You can find my whole solution here

[angelikatyborska]

Sorry for the late reply!

I can see now that your wrong solution (https://github.com/cshintov/exercism/blob/master/elixir/community-garden/lib/community_garden.ex) fails this new test reliably. So I decided to add the new test that you're proposing, but I modified it to incorporate the feedback that I had about it.

@jiegillet Could you take a look if I didn't make any silly mistakes?

[angelikatyborska]

20 * 100ms = 2s, so even is somebody messes up their solution so bad they trigger the timeout every time, they should still see some output of the test. I think the test runner only gets killed by exercism after 3 * average_run_time (which is 3 * 4s in our case)

[jiegillet]

I didn't dive deep into this one, but the added test makes sense to me and looks sensible