search

CoreyKaylor / Lightning.NET

.NET library for LMDB key-value store
Other
397 stars 82 forks source link

Optionally omit closing the database handle, causes issues in multi t… #134

Closed adamfur closed 3 years ago

adamfur commented 3 years ago

Optional behavior to omit calling mdb_dbi_close(), as it causes problems in multi threaded environments. https://github.com/CoreyKaylor/Lightning.NET/issues/72

Also, calling mdb_dbi_close() is normally unnecessary http://www.lmdb.tech/doc/group__mdb.html#ga52dd98d0c542378370cd6b712ff961b5

AlgorithmsAreCool commented 3 years ago

I'm a bit hesitant about this one, why would you want to dispose the LightningDatabase but leave it's handle open? Couldn't you just not dispose the LightningDatabase object?

adamfur commented 3 years ago

"After a successful commit the handle will reside in the shared environment, and may be used by other transactions." http://www.lmdb.tech/doc/group__mdb.html#gac08cad5b096925642ca359a6d6f0562a

Currently if I have multiple running transactions on a single lightning environment, I have to manually keep track of all open lightning databases, and be sure no one closes it prematurely. (can't use "using" and need to track every instance of LightningDatabase so the GC doesn't invoke the finalizers)

With this PR, the client code becomes considerably easier, as database handles are tracked by lmdb environment instead of the client code.

https://github.com/LMDB/lmdb/blob/mdb.master/libraries/liblmdb/mdb.c#L5686 /* Destroy resources from mdb_env_open(), clear our readers & DBIs / static void ESECT mdb_env_close0(MDB_env *env, int excl)

The comment on mdb_env_close() states it clears DBIs.

AlgorithmsAreCool commented 3 years ago

Thinking about it some more, this change is fine as long as we don't change the default behavior. This is technically breaks binary compat by changing the signature, but I don't think nuget packages have to care about that much. I'll merge later today unless @CoreyKaylor objects.

CoreyKaylor commented 3 years ago

This change looks fine to me and would maintain backwards compatibility with existing apps. Thanks for the PR.