rabbit_variable_queue: Ensure modified state is not discarded in ifoldrabbitmq-server-2488

Commit d12b4d8e08005f97c5557f0948b3d03c15ef8c1b introduced a bug where
the old value of `State` is returned in the `stop` case.

The problem was sometimes visible in the backing_queue_SUITE's
`variable_queue_fold` testcase. The testcase crashed with the following
exception:

    === Location: [{rabbit_ct_broker_helpers,rpc,1509},
                  {backing_queue_SUITE,variable_queue_fold,1144},
                  {test_server,ts_tc,1754},
                  {test_server,run_test_case_eval1,1263},
                  {test_server,run_test_case_eval,1195}]
    === === Reason: {error,
                         {case_clause,undefined},
                         [{file_handle_cache,'-partition_handles/1-fun-0-',2,
                              [{file,"src/file_handle_cache.erl"},{line,804}]},
                          {file_handle_cache,get_or_reopen,1,
                              [{file,"src/file_handle_cache.erl"},{line,743}]},
                          {file_handle_cache_stats,timer_tc,1,
                              [{file,"src/file_handle_cache_stats.erl"},
                               {line,54}]},
                          {file_handle_cache_stats,update,2,
                              [{file,"src/file_handle_cache_stats.erl"},
                               {line,40}]},
                          {file_handle_cache,with_handles,3,
                              [{file,"src/file_handle_cache.erl"},{line,700}]},
                          {rabbit_msg_store,read_from_disk,2,
                              [{file,"src/rabbit_msg_store.erl"},{line,1259}]},
                          {rabbit_msg_store,client_read3,3,
                              [{file,"src/rabbit_msg_store.erl"},{line,671}]},
                          {rabbit_msg_store,safe_ets_update_counter,5,
                              [{file,"src/rabbit_msg_store.erl"},{line,1314}]}]}

The `State` (a #vqstate{} record) contains the rabbit_msg_store's state
as well (a #client_msstate{} record). This msg store client state is
updated after most calls to the msg store. In particular, the msg store
client state contains a map in the `file_handle_cache` field of that
record to store all file handles returned by the file_handle_cache.

So each time the msg store opens or closes files as part of its
operation, the client state is updated with a modifed
`file_handle_cache` map.

In addition to that, the file_handle_cache module uses the process
dictionary to store even more data about the various file handles used
by that process.

Therefore, by discarding the updated #client_msstate{}, we loose the
up-to-date `file_handle_cache` map. However, the process dictionary is
still updated by the file_handle_cache module. This means that the map
and the process dictionary are out-of-sync at this point. That's what
causes the crash in the file_handle_cache module: it is called with
arguments which don't work with the data in the process dictionary.

This commit fixes that and re-names some variables to clearly show the
progression of modified data.

Fixes #2488

Major props to @dumbbell for figuring this out, and @pjk25 for assistance!

1 files changed, 10 insertions, 8 deletions

diff --git a/src/rabbit_variable_queue.erl b/src/rabbit_variable_queue.erl
index e3837c726e..cf6fa4a189 100644
--- a/src/rabbit_variable_queue.erl
+++ b/src/rabbit_variable_queue.erl
@@ -2366,21 +2366,23 @@ inext(It, {Its, IndexState}) ->
             {[{MsgStatus1, Unacked, It1} | Its], IndexState1}
     end.
 
-ifold(_Fun, Acc, [], State) ->
-    {Acc, State};
-ifold(Fun, Acc, Its, State) ->
+ifold(_Fun, Acc, [], State0) ->
+    {Acc, State0};
+ifold(Fun, Acc, Its0, State0) ->
     [{MsgStatus, Unacked, It} | Rest] =
         lists:sort(fun ({#msg_status{seq_id = SeqId1}, _, _},
                         {#msg_status{seq_id = SeqId2}, _, _}) ->
                            SeqId1 =< SeqId2
-                   end, Its),
-    {Msg, State1} = read_msg(MsgStatus, State),
+                   end, Its0),
+    {Msg, State1} = read_msg(MsgStatus, State0),
     case Fun(Msg, MsgStatus#msg_status.msg_props, Unacked, Acc) of
         {stop, Acc1} ->
-            {Acc1, State};
+            {Acc1, State1};
         {cont, Acc1} ->
-            {Its1, IndexState1} = inext(It, {Rest, State1#vqstate.index_state}),
-            ifold(Fun, Acc1, Its1, State1#vqstate{index_state = IndexState1})
+            IndexState0 = State1#vqstate.index_state,
+            {Its1, IndexState1} = inext(It, {Rest, IndexState0}),
+            State2 = State1#vqstate{index_state = IndexState1},
+            ifold(Fun, Acc1, Its1, State2)
     end.
 
 %%----------------------------------------------------------------------------