salortiz / LMDB_File

Perl wrapper around the OpenLDAP's LMDB
Other
8 stars 12 forks source link

MDB_NOTFOUND results in croak when using low-level txn object #18

Open akotlar opened 8 years ago

akotlar commented 8 years ago

Line 383 of LMDB_File.pm shows the expected behavior, no croak:

This is a bug because it subverts the consistency expected from get operations. Either both the high-level and low-level interface should croak, or neither. I vote neither, because in my experience dying for MDB_NOTFOUND is never the behavior I want; when data is not found I know it by less destructive means.

sub get {
    warn "get: '$_[1]'\n" if $DEBUG > 2;
    my($txn, $dbi) = _chkalive($_[0]);
    return _get($txn, $dbi, $_[1], $_[2]) if @_ > 2;
    my($res, $data);
    {
    local $die_on_err = 0;
    $res = _get($txn, $dbi, $_[1], $data);
    }
    croak($@) if $res && $die_on_err && $res != MDB_NOTFOUND() or undef $@;
    $data;
}
hoytech commented 8 years ago

I agree they should be consistent and if not that is a bug...

Throwing from MDB_NOTFOUND is also quite annoying for all my use-cases but at least it's consistent. I'm for changing it but we might need to be careful for backwards compat (maybe when $die_on_err == 2 then it should croak for everything except NOTFOUND?).

akotlar commented 8 years ago

@hoytech I agree, backward compatibility may be an issue. I think solving that with different die_on_err levels could be a good solution. We could set die_on_err = 1 to be the default behavior, as you said, and then $die_on_error == 2 for everything MDB_NOTFOUND, and MDB_KEYEXST when MDB_NOOVERWRITE and MDB_NODUPDATA are not set.

I need to confirm that MDB_KEYEXIST also croaks the program when MDB_NOOVERWRITE or MDB_NODUPDATA is not set (using the low level txn object).