From 57b712203570fd16b50c1689456c15df6ffa791d Mon Sep 17 00:00:00 2001 From: Scott Lystig Fritchie Date: Sat, 18 Jul 2015 23:22:14 +0900 Subject: [PATCH] Fix bug found by PULSE that's not directly chain manager-related (more) PULSE managed to create a situation where machi_proxy_flu_client1 would appear to fail a remote attempt to write_projection. The client would retry, but the 1st attempt really did get through to the server. So, if we hit this case, we try to read the projection, and if it's exactly equal to what we tried to write, we consider the op a success. Ditto for write_chunk. Fix up eunit test to accomodate the change of semantics. --- src/machi_proxy_flu1_client.erl | 37 +++++++++++++++++++++++--- test/machi_proxy_flu1_client_test.erl | 38 ++++++++++++++++++++++----- 2 files changed, 64 insertions(+), 11 deletions(-) diff --git a/src/machi_proxy_flu1_client.erl b/src/machi_proxy_flu1_client.erl index eb3adf8..bef5ad6 100644 --- a/src/machi_proxy_flu1_client.erl +++ b/src/machi_proxy_flu1_client.erl @@ -223,8 +223,26 @@ write_projection(PidSpec, ProjType, Proj) -> %% @doc Write a projection `Proj' of type `ProjType'. write_projection(PidSpec, ProjType, Proj, Timeout) -> - gen_server:call(PidSpec, {req, {write_projection, ProjType, Proj}}, - Timeout). + case gen_server:call(PidSpec, {req, {write_projection, ProjType, Proj}}, + Timeout) of + {error, written}=Err -> + Epoch = Proj#projection_v1.epoch_number, + case read_projection(PidSpec, ProjType, Epoch, Timeout) of + {ok, Proj2} when Proj2 == Proj -> + %% The proxy made (at least) two attempts to write + %% this projection. An earlier one appeared to + %% have failed, so the proxy retried. The later + %% attempt returned to us {error,written} because + %% the earlier attempt was actually received & + %% processed by the server. So, we consider this + %% a successful write. + ok; + _ -> + Err + end; + Else -> + Else + end. %% @doc Get all projections from the FLU's projection store. @@ -277,8 +295,19 @@ write_chunk(PidSpec, EpochID, File, Offset, Chunk) -> %% with `Prefix' at `Offset'. write_chunk(PidSpec, EpochID, File, Offset, Chunk, Timeout) -> - gen_server:call(PidSpec, {req, {write_chunk, EpochID, File, Offset, Chunk}}, - Timeout). + case gen_server:call(PidSpec, {req, {write_chunk, EpochID, File, Offset, Chunk}}, + Timeout) of + {error, written}=Err -> + case read_chunk(PidSpec, EpochID, File, Offset, Chunk, Timeout) of + {ok, Chunk2} when Chunk2 == Chunk -> + %% See equivalent comment inside write_projection(). + ok; + _ -> + Err + end; + Else -> + Else + end. %%%%%%%%%%%%%%%%%%%%%%%%%%% diff --git a/test/machi_proxy_flu1_client_test.erl b/test/machi_proxy_flu1_client_test.erl index 8550384..aba5612 100644 --- a/test/machi_proxy_flu1_client_test.erl +++ b/test/machi_proxy_flu1_client_test.erl @@ -121,12 +121,14 @@ flu_restart_test() -> {ok, Prox1} = ?MUT:start_link(I), try FakeEpoch = ?DUMMY_PV1_EPOCH, - Data = <<"data!">>, + Data = <<"data!">>, + Dataxx = <<"Fake!">>, {ok, {Off1,Size1,File1}} = ?MUT:append_chunk(Prox1, FakeEpoch, <<"prefix">>, Data, infinity), P_a = #p_srvr{name=a, address="localhost", port=6622}, - P1 = machi_projection:new(1, a, [P_a], [], [a], [], []), + P1 = machi_projection:new(1, a, [P_a], [], [a], [], []), + P1xx = P1#projection_v1{dbg2=["not exactly the same as P1!!!"]}, EpochID = {P1#projection_v1.epoch_number, P1#projection_v1.epoch_csum}, ok = ?MUT:write_projection(Prox1, public, P1), @@ -182,18 +184,31 @@ flu_restart_test() -> (line) -> io:format("line ~p, ", [?LINE]); (stop) -> ?MUT:read_projection(Prox1, private, 7) end, - fun(run) -> {error, written} = + fun(run) -> ok = ?MUT:write_projection(Prox1, public, P1), ok; (line) -> io:format("line ~p, ", [?LINE]); (stop) -> ?MUT:write_projection(Prox1, public, P1) end, - fun(run) -> {error, written} = + fun(run) -> ok = ?MUT:write_projection(Prox1, private, P1), ok; (line) -> io:format("line ~p, ", [?LINE]); (stop) -> ?MUT:write_projection(Prox1, private, P1) end, + fun(run) -> {error, written} = + ?MUT:write_projection(Prox1, public, P1xx), + ok; + (line) -> io:format("line ~p, ", [?LINE]); + (stop) -> ?MUT:write_projection(Prox1, public, P1xx) + end, + fun(run) -> {error, written} = + ?MUT:write_projection(Prox1, private, P1xx), + ok; + (line) -> io:format("line ~p, ", [?LINE]); + (stop) -> ?MUT:write_projection(Prox1, private, P1xx) + end, + fun(run) -> {ok, [_]} = ?MUT:get_all_projections(Prox1, public), ok; @@ -249,9 +264,7 @@ flu_restart_test() -> (line) -> io:format("line ~p, ", [?LINE]); (stop) -> ?MUT:wedge_status(Prox1) end, - %% NOTE: When write-once enforcement is enabled, this test - %% will fail: change ok -> {error, written} - fun(run) -> %% {error, written} = + fun(run) -> ok = ?MUT:write_chunk(Prox1, FakeEpoch, File1, Off1, Data, infinity), @@ -259,6 +272,17 @@ flu_restart_test() -> (line) -> io:format("line ~p, ", [?LINE]); (stop) -> ?MUT:write_chunk(Prox1, FakeEpoch, File1, Off1, Data, infinity) + end, + %% NOTE: When write-once enforcement is enabled, this test + %% will fail: change ok -> {error, written} + fun(run) -> %% {error, written} = + ok = + ?MUT:write_chunk(Prox1, FakeEpoch, File1, Off1, + Dataxx, infinity), + ok; + (line) -> io:format("line ~p, ", [?LINE]); + (stop) -> ?MUT:write_chunk(Prox1, FakeEpoch, File1, Off1, + Dataxx, infinity) end ],