Proper handling of bitcask_fileops:fold/fold_keys errors #31

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

Contributor
hdima commented Feb 4, 2012

Currently errors from bitcask_fileops:fold/fold_keys can be masked by a some other error or just ignored in the bitcask module. The patch fixes that by throw errors in bitcask_fileops:fold/fold_keys instead of returning {error, Reason}.

hdima added some commits Feb 4, 2012
@hdima hdima Throw errors in bitcask_fileops:fold/3 instead of returning {error, R…
…eason}.

Errors returned from bitcask_fileops:fold/3 didn't checked by the client
code in the bitcask module and could be masked by a some other exception.
87c846b
@hdima hdima Throw errors in bitcask_fileops:fold_keys() instead of returning {err…
…or, Reason}.

Errors returned from bitcask_fileops:fold_keys() were just ignored by the client
code in the bitcask module.
fe21331
Contributor

I'm soliciting Dizzy's opinion now. I think this looks good and would like
to merge this today.

Contributor

I glanced over it a while back and the intent looked very good to me - I just didn't have time to thoroughly go through each case and evaluate consequences of the changes in the Riak backend.

Contributor

So, I checked out origin/bz1160-eunit-test, then added Dmitry's eunit test (appends a NUL byte to the end of the bitcask file to anger the checksum check), then added the two lines at the end:

FoldFun = fun (_K, _V, Acc) ->
    io:format(user, "~p, ~p, ~p\n", [_K, _V, Acc]),
    Acc + 1
end,
[...]
XX = (catch bitcask:fold(B2, FoldFun, 0)),
io:format(user, "\nXX = ~p\n", [XX]),

It outputs this:

<<"k1">>, <<"?">>, {error,{bad_crc,<<>>,0}}

XX = {'EXIT',{badarith,[{bitcask,'-fold_error_handling_test/0-fun-0-',3},
                    {bitcask_fileops,fold_loop,7},
                    {bitcask,subfold,3},
                    {bitcask_nifs,keydir_frozen,4},
                    {bitcask,fold_error_handling_test,0},
                    {eunit_test,'-function_wrapper/2-fun-0-',2},
                    {eunit_test,run_testfun,1},
                    {eunit_proc,run_test,1}]}}

So, even on the 1160-eunit-test branch, the fold is allowed to continue with a now-bogus accumulator. The folding fun could've been looking for a 2-tuple and done something with the faulty accumulator that could propagate bogus data many fold fun calls into the future.

Contributor

Howdy, sorry about the long delay ... I'm closing this one due to superceding by #64, which fixes this and one other fold-related bug and changes the behavior slightly on bad CRC, i.e. skip the bad item & continue the fold rather than stopping.

@slfritchie slfritchie closed this Oct 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment