CoreyKaylor / Lightning.NET

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

Fixed bug with Compare delegate lifetime #47

Closed mjp426 closed 9 years ago

mjp426 commented 9 years ago

Hi, thanks for the great project!

I've found a bug using custom Compare functions. The DatabaseOptions class created a delegate to perform the comparison but it doesn't store a reference so the delegate doesn't survive garbage collection. I've created a new test that forces garbage collection to show the problem. I've fixed it by moving the SetComparer() logic into the LightningDatabase class and kept a reference to the delegate in the LightningDatabase object.

I've tried to keep everything matching the current style of the project. Let me know what you think of the fix.

Thanks

ilyalukyanov commented 9 years ago

Hi, thank you for the fix, however I don't see a GC problem. Every comparer lives for as long as a transaction it's set in does. To achieve this a reference to the comparer is stored in dictionary tran.SubTransactionsManager.StoreComparer(comparer);.

I've run your test against the unchanged code and it passed. I also tried to call GC.Collect() multiple times to make sure two generations of objects were affected.

mjp426 commented 9 years ago

Ah I see, then my refactoring is overkill. But you need to keep a reference to compareFunction rather than comparer as it's compareFunction that gets passed into the native code logic and is getting garbage collected.

I agree that the test is not reliable. I have it failing on one machine but working on another. I get it to fail consistently by creating 100,000 unreferenced objects immediately before I call GC.Collect(). I can commit an updated test and (simpler) fix at some point, but hopefully my description above makes sense?

ilyalukyanov commented 9 years ago

Yeah, makes total sense. Indeed compareFnuction should be stored. Will fix it.