From 336b21ef31a838c1d9c4a991cd57d7efd91b86ed Mon Sep 17 00:00:00 2001 From: Rick Carlino Date: Mon, 18 May 2020 09:16:35 -0500 Subject: [PATCH] Documentation and test fixes. --- .../compiler_groups_test.exs | 4 +- .../farmbot_celery_script/compiler_test.exs | 97 ++++++++++++++----- .../lib/farmbot_ext/api/dirty_worker.ex | 70 ++++++++----- 3 files changed, 121 insertions(+), 50 deletions(-) diff --git a/farmbot_celery_script/test/farmbot_celery_script/compiler_groups_test.exs b/farmbot_celery_script/test/farmbot_celery_script/compiler_groups_test.exs index 3ac9645a..91e8c66c 100644 --- a/farmbot_celery_script/test/farmbot_celery_script/compiler_groups_test.exs +++ b/farmbot_celery_script/test/farmbot_celery_script/compiler_groups_test.exs @@ -47,8 +47,8 @@ defmodule FarmbotCeleryScript.CompilerGroupsTest do canary_actual = :crypto.hash(:sha, Macro.to_string(result)) canary_expected = - <<157, 69, 5, 38, 188, 78, 10, 183, 154, 99, 151, 193, 214, 208, 187, 130, - 183, 73, 13, 48>> + <<136, 140, 48, 226, 216, 155, 178, 103, 244, 88, 225, 146, 130, 216, 125, + 72, 113, 195, 65, 1>> # READ THE NOTE ABOVE IF THIS TEST FAILS!!! assert canary_expected == canary_actual diff --git a/farmbot_celery_script/test/farmbot_celery_script/compiler_test.exs b/farmbot_celery_script/test/farmbot_celery_script/compiler_test.exs index ee8d480d..7ecf3c44 100644 --- a/farmbot_celery_script/test/farmbot_celery_script/compiler_test.exs +++ b/farmbot_celery_script/test/farmbot_celery_script/compiler_test.exs @@ -79,7 +79,7 @@ defmodule FarmbotCeleryScript.CompilerTest do end test "identifier sanitization" do - label = "System.cmd(\"rm\", [\"-rf /*\"])" + label = "System.cmd(\"echo\", [\"lol\"])" value_ast = AST.Factory.new("coordinate", x: 1, y: 1, z: 1) identifier_ast = AST.Factory.new("identifier", label: label) @@ -120,11 +120,19 @@ defmodule FarmbotCeleryScript.CompilerTest do [ fn params -> _ = inspect(params) + unsafe_U3lzdGVtLmNtZCgiZWNobyIsIFsibG9sIl0p = FarmbotCeleryScript.SysCalls.coordinate(1, 1, 1) - #{var_name} = - FarmbotCeleryScript.SysCalls.coordinate(1, 1, 1) + better_params = %{ + "System.cmd(\\"echo\\", [\\"lol\\"])" => %FarmbotCeleryScript.AST{ + args: %{x: 1, y: 1, z: 1}, + body: [], + comment: nil, + kind: :coordinate, + meta: nil + } + } - [fn -> #{var_name} end] + [fn -> unsafe_U3lzdGVtLmNtZCgiZWNobyIsIFsibG9sIl0p end] end ] """) @@ -372,19 +380,38 @@ defmodule FarmbotCeleryScript.CompilerTest do unsafe_cGFyZW50 = Keyword.get(params, :unsafe_cGFyZW50, FarmbotCeleryScript.SysCalls.coordinate(1, 2, 3)) + better_params = %{} + [ fn -> - FarmbotCeleryScript.Compiler.UpdateResource.do_update( - %FarmbotCeleryScript.AST{ - args: %{label: "parent"}, - body: [], - comment: nil, - kind: :identifier, - meta: nil - }, - %{"plant_stage" => "removed"}, - [] - ) + me = FarmbotCeleryScript.Compiler.UpdateResource + + variable = %FarmbotCeleryScript.AST{ + args: %{label: "parent"}, + body: [], + comment: nil, + kind: :identifier, + meta: nil + } + + update = %{"plant_stage" => "removed"} + + case(variable) do + %AST{kind: :identifier} -> + args = Map.fetch!(variable, :args) + label = Map.fetch!(args, :label) + resource = Map.fetch!(better_params, label) + me.do_update(resource, update) + + %AST{kind: :point} -> + me.do_update(variable.args(), update) + + %AST{kind: :resource} -> + me.do_update(variable.args(), update) + + res -> + raise("Resource error. Please notfiy support: \#{inspect(res)}") + end end ] end @@ -405,20 +432,38 @@ defmodule FarmbotCeleryScript.CompilerTest do [ fn params -> _ = inspect(params) + better_params = %{} [ fn -> - FarmbotCeleryScript.Compiler.UpdateResource.do_update( - %FarmbotCeleryScript.AST{ - args: %{resource_id: 23, resource_type: "Plant"}, - body: [], - comment: nil, - kind: :resource, - meta: nil - }, - %{"plant_stage" => "planted", "r" => 23}, - [] - ) + me = FarmbotCeleryScript.Compiler.UpdateResource + + variable = %FarmbotCeleryScript.AST{ + args: %{resource_id: 23, resource_type: "Plant"}, + body: [], + comment: nil, + kind: :resource, + meta: nil + } + + update = %{"plant_stage" => "planted", "r" => 23} + + case(variable) do + %AST{kind: :identifier} -> + args = Map.fetch!(variable, :args) + label = Map.fetch!(args, :label) + resource = Map.fetch!(better_params, label) + me.do_update(resource, update) + + %AST{kind: :point} -> + me.do_update(variable.args(), update) + + %AST{kind: :resource} -> + me.do_update(variable.args(), update) + + res -> + raise("Resource error. Please notfiy support: \#{inspect(res)}") + end end ] end diff --git a/farmbot_ext/lib/farmbot_ext/api/dirty_worker.ex b/farmbot_ext/lib/farmbot_ext/api/dirty_worker.ex index af55208e..0aa45140 100644 --- a/farmbot_ext/lib/farmbot_ext/api/dirty_worker.ex +++ b/farmbot_ext/lib/farmbot_ext/api/dirty_worker.ex @@ -8,7 +8,7 @@ defmodule FarmbotExt.API.DirtyWorker do require Logger require FarmbotCore.Logger use GenServer - @timeout 1 + @timeout 500 # these resources can't be accessed by `id`. @singular [ @@ -42,33 +42,21 @@ defmodule FarmbotExt.API.DirtyWorker do @impl GenServer def handle_info(:do_work, %{module: module} = state) do - Process.sleep(1000) - list = Enum.uniq((Private.list_dirty(module) ++ Private.list_local(module))) + Process.sleep(@timeout) + list = Enum.uniq(Private.list_dirty(module) ++ Private.list_local(module)) - stale = Enum.find(list, fn item -> - if (item.id) do - if item == Repo.get_by(module, id: item.id) do - false - else - IO.inspect(item, label: "=== STALE RECORD DETECTED!") - true - end - else - false - end - end) - - if stale do - Process.send_after(self(), :do_work, @timeout) - {:noreply, state} - else + unless has_race_condition?(module, list) do Enum.map(list, fn dirty -> work(dirty, module) end) - Process.send_after(self(), :do_work, @timeout) - {:noreply, state} end + + Process.send_after(self(), :do_work, @timeout) + {:noreply, state} end def work(dirty, module) do + # Go easy on the API + Process.sleep(333) + case http_request(dirty, module) do # Valid data {:ok, %{status: s, body: body}} when s > 199 and s < 300 -> @@ -139,4 +127,42 @@ defmodule FarmbotExt.API.DirtyWorker do data = render(module, dirty) API.patch(API.client(), path, data) end + + # This is a fix for a race condtion. The root cause is unknown + # as of 18 May 2020. The problem is that records are marked + # diry _before_ the dirty data is saved. That means that FBOS + # knows a record has changed, but for a brief moment, it only + # has the old copy of the record (not the changes). + # Because of this race condtion, + # The race condition: + # + # * Is nondeterministic + # * Happens frequently when running many MARK AS steps in one go. + # * Happens frequently when Erlang VM only has one thread + # * Ie: `iex --erl '+S 1 +A 1' -S mix` + # * Happens frequently when @timeout is decreased to `1`. + # + # This function PREVENTS CORRUPTION OF API DATA. It can be + # removed once the root cause of the data race is determined. + # - RC 18 May 2020 + def has_race_condition?(module, list) do + Enum.find(list, fn item -> + if item.id do + if item == Repo.get_by(module, id: item.id) do + # This item is OK - no race condition. + false + else + # There was a race condtion. We probably can't trust + # any of the data in this list. We need to wait and + # try again later. + Process.sleep(@timeout * 3) + true + end + else + # This item only exists on the FBOS side. + # It will never be affected by the data race condtion. + false + end + end) + end end