{"action":"created","issue":{"url":"https://api.github.com/repos/basho/riak_kv/issues/1155","labels_url":"https://api.github.com/repos/basho/riak_kv/issues/1155/labels{/name}","comments_url":"https://api.github.com/repos/basho/riak_kv/issues/1155/comments","events_url":"https://api.github.com/repos/basho/riak_kv/issues/1155/events","html_url":"https://github.com/basho/riak_kv/pull/1155","id":97141633,"number":1155,"title":"Converting stop_fold exception breaks vnode worker","user":{"login":"engelsanchez","id":115947,"avatar_url":"https://avatars.githubusercontent.com/u/115947?v=3","gravatar_id":"","url":"https://api.github.com/users/engelsanchez","html_url":"https://github.com/engelsanchez","followers_url":"https://api.github.com/users/engelsanchez/followers","following_url":"https://api.github.com/users/engelsanchez/following{/other_user}","gists_url":"https://api.github.com/users/engelsanchez/gists{/gist_id}","starred_url":"https://api.github.com/users/engelsanchez/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/engelsanchez/subscriptions","organizations_url":"https://api.github.com/users/engelsanchez/orgs","repos_url":"https://api.github.com/users/engelsanchez/repos","events_url":"https://api.github.com/users/engelsanchez/events{/privacy}","received_events_url":"https://api.github.com/users/engelsanchez/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":4,"created_at":"2015-07-24T20:35:47Z","updated_at":"2015-08-05T20:35:53Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/basho/riak_kv/pulls/1155","html_url":"https://github.com/basho/riak_kv/pull/1155","diff_url":"https://github.com/basho/riak_kv/pull/1155.diff","patch_url":"https://github.com/basho/riak_kv/pull/1155.patch"},"body":"This fixes a problem where, occasionally, paginated 2i queries would be missing values.  The fix is very short, but the explanation is very long:\r\n\r\n2i queries eventually execute folds, which run in one of the the riak_kv vnode pool workers.  The request to run work in a pool worker has a fold function with an accumulator, and a finish function that runs on the result of the fold function ([riak_kv_worker:handle_work](https://github.com/basho/riak_kv/blob/ec9b41ece5e5fe2ce6d6b30cfe43c2cbc93db917/src/riak_kv_worker.erl#L44)). See how a stop_fold exception is handled by skipping the finish function.  In the case of paginated 2i queries, the fold sends back results to the index FSM using [riak_kv_vnode:result_fun_ack](https://github.com/basho/riak_kv/blob/ec9b41ece5e5fe2ce6d6b30cfe43c2cbc93db917/src/riak_kv_vnode.erl#L1714) (see [2i handler](https://github.com/basho/riak_kv/blob/ec9b41ece5e5fe2ce6d6b30cfe43c2cbc93db917/src/riak_kv_vnode.erl#L821)), which waits for an ack from the index FSM and may throw stop_fold if the FSM signals it does not need any more results.\r\n\r\nResults are collected in batches before being sent in a [riak_kv_fold_buffer](https://github.com/basho/riak_kv/blob/ec9b41ece5e5fe2ce6d6b30cfe43c2cbc93db917/src/riak_kv_vnode.erl#L895), which is used as the accumulator in the fold.  Adding to this buffer and reaching the batch limit is what triggers the sending.  A [finish function](https://github.com/basho/riak_kv/blob/ec9b41ece5e5fe2ce6d6b30cfe43c2cbc93db917/src/riak_kv_vnode.erl#L1741-L1749) is provided so that any remaining items in the buffer by the end of the fold are also sent.  That's the key of the problem right there: the folder adds an item, the buffer gets full, the folder sends the batch to the FSM and receives a stop command, then throws stop_fold.  The intention is to stop the fold right there and not send any more items, so the finish function should be skipped. The stoppable_fold wrapper was catching that stop_fold and converting it to a {break, Acc} exception, where the accumulator was the value **before** the last items were sent, still containing most of those items.  The riak_kv_fold_worker never gets the exception, and simply processes that last buffer minus last item by calling the finish function on it, re-sending those items. An example:\r\n\r\n* Last batch, after adding last item 9: [7,8,9]\r\n* FSM receives [7,8,9]. Then, if it stays around long enough, it receives an extra [7,8] due to the finish function being called on the stale accumulator and ends up with [7,8,9,7,8].  **This is the bit that is timing sensitive and makes it non-deterministic**.\r\n* The merge sort algorithm expects items in order. The result is undefined otherwise. So occasionally it will output items out of order. For example [7,8,7,9,8].  It also truncates to page size, so the [9,8] is dropped and the 7 dropped due to de-duplication.\r\n\r\n/cc @jonmeredith @JeetKunDoug\r\nThe code around the fold functions is pretty hairy. Let me know if you need more clarification (you will)."},"comment":{"url":"https://api.github.com/repos/basho/riak_kv/issues/comments/128140509","html_url":"https://github.com/basho/riak_kv/pull/1155#issuecomment-128140509","issue_url":"https://api.github.com/repos/basho/riak_kv/issues/1155","id":128140509,"user":{"login":"jonmeredith","id":64318,"avatar_url":"https://avatars.githubusercontent.com/u/64318?v=3","gravatar_id":"","url":"https://api.github.com/users/jonmeredith","html_url":"https://github.com/jonmeredith","followers_url":"https://api.github.com/users/jonmeredith/followers","following_url":"https://api.github.com/users/jonmeredith/following{/other_user}","gists_url":"https://api.github.com/users/jonmeredith/gists{/gist_id}","starred_url":"https://api.github.com/users/jonmeredith/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jonmeredith/subscriptions","organizations_url":"https://api.github.com/users/jonmeredith/orgs","repos_url":"https://api.github.com/users/jonmeredith/repos","events_url":"https://api.github.com/users/jonmeredith/events{/privacy}","received_events_url":"https://api.github.com/users/jonmeredith/received_events","type":"User","site_admin":false},"created_at":"2015-08-05T20:35:53Z","updated_at":"2015-08-05T20:35:53Z","body":"+1 9b82812\r\n\r\nMentally walked through the code.\r\nRan the EQC riak_test (at commit 2.0.0-234-gfa49385)\r\nRan the 'repro.erl' script that replicated the original problem and was resolved."}}