search

erthink / libmdbx

One of the fastest embeddable key-value ACID database without WAL. libmdbx surpasses the legendary LMDB in terms of reliability, features and performance.
https://erthink.github.io/libmdbx/
Other
1.16k stars 110 forks source link

Add move assignment operator for cursor_managed #270

Closed canepat closed 2 years ago

canepat commented 2 years ago

Custom move assignment operator for cursor_managed is required to close the current handle before moving (see also https://github.com/torquem-ch/silkworm/issues/575).

erthink commented 2 years ago

Hmm, I will fix this (by my self) promptly.

canepat commented 2 years ago

Sorry, I've missed that cursor explicitly manages the handle, so probably the correct solution is a bit more involved.

erthink commented 2 years ago

Please check out the https://github.com/erthink/libmdbx/commit/d8431a35cb17cc052c1d6b29749416a6bbfb589f

erthink commented 2 years ago

Minor refine commit' comment by the https://github.com/erthink/libmdbx/commit/3c574fca99a5c2882780031f4b4d887e8bfae218.

erthink commented 2 years ago

@canepat, thank you for reporting.

canepat commented 2 years ago

These changes raise now compiler errors on code using mdbx in cases like this:

mdbx::txn tx = ...
mdbx::cursor_managed source{tx.open_cursor(...)};
...
source.close();
source = tx.open_cursor(...); // compiler error here

which compiled and worked as expected before (the leak was present just if close is not called). I don't think that your goal is prohibiting the cursor_managed move assignment or am I missing something?

Apart from compiler errors, using the inherited move assignment operator in cursor_managed class seems insufficient to me because the base class operator does not dispose the handle. Isn't the cursor_managed class supposed to manage the handle implicitly in all cases?

The change proposed in this PR doesn't break the case listed above and also closes the handle automatically when moving if close has not been called explicitly.

canepat commented 2 years ago

I've seen now your rework in https://github.com/erthink/libmdbx/commit/464886ab614378cc7036cdd047c6b64adabb8930 and it looks good to me, it should solve my issues in previous comment.