Add support for Merge

[mkosieradzki]

@warrenfalk
I have created a prototype for merge operation handling (basically just expanded your:
rocksdb_mergeoperator_create and rocksdb_mergeoperator_destroy
and added proper declarations for delegates)

I've got couple of questions before submiting a PR:

1. Should I remove ROCKSDB_MERGE_OPERATOR constant if I create a full low-level implementation
2. Should I start with low-level + tests or you expect a full high-level implementation?

I also understand you expect at least a functional smoke test for this feature.

[warrenfalk]

Yes, removing the compile-time constants as implementations are added is fine. Low level without high level is fine.

[mkosieradzki]

I have created PR.

I don't like one thing about existing code, specifically cleanup functions in destructors. This is not the best idea especially you cannot predict when destructor will be called (it can happen after destroying the runtime). And I see you are already having some issues related to those destructors...

My recommendation would be to use IDisposable objects whenever cleanup is useful.

Current pattern with destructor cannot work because after destroying rocksdb runtime you have no means to check whether handle is still valid (checking it against IntPtr.Zero makes no real sense in that context...)

Also I have noticed that you were struggling with objects being deallocated. My recommendation here would be to use more GCHandles (You are using one already for object pinning) but there are two another uses for GCHandles:

1. You can make sure object is not garbage collected until GCHandle keeping object is not freed.
2. You can pass handle to the native runtime and get it back (even without pinning!). It is a common interop pattern to pass GCHandle as a pointer and in callback use it for dereferencing managed class.

[warrenfalk]

You'll have to be more specific about memory. There are no current issues with memory allocations and in fact object lifetime seems to be functioning perfectly. It neither leaks nor double-frees. It is easy to use. It is efficient.

With respect to IDisposable:

There is no real cleanup on most of these with the "create/destroy" pattern. Since RocksDb is exposed through the C API, there is often creation of what is no more than a heap-allocated structure. The functions to "destroy" there are only freeing memory. The solution I have in place elegantly allows the GC to handle that. I use IDisposable already where there is real cleanup or where allocation/free has to be deterministic.

This is important because using IDisposable in all cases requires the developer to manage manually what can be done automatically, and again, it is happening automatically now and there are no issues. Requiring it to be manual is also another potential failure point.

My system is basically that if a managed object A is tracking an unmanaged object B and a managed object X is tracking unmanaged object Y, and if B internally retains a pointer to Y in unmanaged code, then I will make A retain a managed reference to X in code to mirror this. This allows the GC to just do what it is designed to do and it does it flawlessly.

Also I have noticed that you were struggling with objects being deallocated

No, the only struggle is in figuring out how rocksdb expects object ownership to work in all cases (e.g. when it copies, when it references, and who deallocates). Also IIRC, the rocksdb C API sometimes adds heap allocation where the C++ interface would normally just use stack allocation thus adding extra calls to free objects which aren't well documented in the C++ api. To help debug this, I do have the ability to turn off destroys during debugging with a compiler option.

Yes, the IntPtr.Zero code has no actual effect in most cases and is only there as a guard against future code changes which may deallocate an object before a destructor runs (e.g. if implementing IDisposable) or as a guard in case a destructor runs twice. None of these objects are created and freed frequently, so performance is not suffering here.

I am not sure how you are suggesting GC handle fit into this. I don't see how either of those other uses of GC Handles applies here. Maybe you can elaborate. If you want to try to rewrite allocation/deallocation, submit a PR and I'll take a look.

[mkosieradzki]

@warrenfalk
Regarding GCHandle - my PR uses GCHandle while passing objects to the C API - so you can see this in my PR.

Regarding destructors. If you enable break on all exceptions in VS, then when running tests sometimes destructors fail because they make double free. Disposing rocksdb runtime frees all the managed structures and then you make one more free from the destructor.

At least there should be a full IDisposable+finalizer pattern giving chance to do proper disposal when developer wants. Using only GC for that is just pure evil and should never happen.

[mkosieradzki]

Maybe it would be a good idea to use SafeHandle for all those unmanaged objects?

[warrenfalk]

Your PR uses GCHandle because it has to pass a managed handle to the runtime. I'm already using GCHandle for the same purpose. These are managed handles in unmanaged code. But above we were talking about unmanaged handles in managed code and my question was how GC handle fit into that.

I would need to see an example of how enabling break on all exceptions causes a double free. I don't see any way this is possible right now.

At least there should be a full IDisposable+finalizer pattern giving chance to do proper disposal when developer wants. Using only GC for that is just pure evil and should never happen.

You should take the time to understand what I'm saying. I already use IDisposable when proper disposal is necessary. I only rely on GC when the destruction just means "free the memory". Implementing IDisposable in these so that you can free that native memory when you want to is pure insanity in most cases because 1) you are only freeing the native memory, the GC will free the managed memory when it gets around to it and 2) it hints to the developer that there is more than deallocation happening on disposal when there isn't and 3) it opens the developer up to do it wrong and free while still in use.

I recognize that you may not be an expert in interop. But you seem to be failing to distinguish between what is just a native pointer to memory and what is a native operating system resource handle. The difference between these things is precisely why there is an IDisposable and why it isn't implemented on all classes. Are you aware of some use of SafeHandle that solves a problem here? If so, present exactly what you are suggesting here so we can assess it.

[mkosieradzki]

OK. First of all regarding the double free issue - I was wrong here and I admit it: I have rechecked this and it's a problem with MergeOperator lifetime (it was a problem with freeing options which held a reference to not disposed mergeoperator.). I need to analyze it carefuly. My apologies.

Regarding SafeHandle - it should technically work with both pointers and operating system handles, but it seems to be a bit overkill for "free" as you have mentioned.

I wrote that "you are struggling", because there are tons of NO_DESTROY fragments.

I also tend to disagree that this is a simple free.
For example DbOptions class is using finalizers for cleanup. And when you associate a merge operator it causes this issue: rocksdb_options_destroy cannot be called before rocksdb_mergeoperator_destroy is called on the nested options.

And this is probably only one of many scenarios which require controlling of "destroy" order.

How would you suggest handling the case with merge operator using destructors?

[warrenfalk]

The #if !NODESTROY blocks just provide a simple way to turn off destruction for debugging purposes, you shouldn't read too much into it.

The question really is why the one destroy() can't be called before the other. And the reason probably is because the one object has an internal reference to the other. So then this isn't a scenario which requires controlling of destroy order. GCs already control destroy order based on what has a reference to what. You just need to communicate this information to the GC because it doesn't know about native references to native objects. So the way I do this is to make sure that the managed wrappers have references to managed wrappers which mirror internal references. For example, look at the Iterator class. You need to supply a ReadOptions to use the Iterator, and the Iterator internally is keeping a reference to the native ReadOptions, so you can't destroy it until you are done with the Iterator. I don't have an IDisposable on ReadOptions. Instead I have the managed Iterator retain a reference to the managed ReadOptions. Now the GC knows what references what and which must outlive the other. I haven't been through your PR code yet entirely, but I suspect you need to do something similar.

Also as a side note: Notice how Iterator implements IDisposable. This is because it has a real cleanup on dispose which frees resources in the database. ReadOptions, on the other hand, is just a glorified in-memory data structure.

[mkosieradzki]

OK. Thanks, I will correct my PR accordingly.

[mkosieradzki]

In general I am definitely used to dealing with unmanaged resources using IDisposable and having everything under control, but you have a completely different approach (I understand your points, but I definitely don't know how to integrate with your approach).

I think that the difference in our approaches lead to many misunderstandings in this discussion. For example I like using GCHandles because it enables binding of object life-times to the native ones (it will not be collected unless explicitely instructed to), but it requires using a lot of dispose etc. This is definitely my approach. You on the contrary try to use GC as much as possible and make very easy to use library for C# developers and GCHandles are counter-productive here.

I would be grateful if you fixed my PR to match your style. It would be an interesting case to solve for me and in that case I will probably still be able to add some contributions soon.

I am definitely stopping to push you towards going with the IDisposable-style, because its a very decent goal to create a C# developer-friendly library and I've noticed you put a lot of effort here and wish you the very best luck.

[warrenfalk]

I will take a look later. It's possible that what you're running into is a confusing ownership model that I alluded to above. It is possible that the options instance is calling destroy on any merge operators associated with it, and thus destroying the merge operator yourself is double freeing.

[mkosieradzki]

@warrenfalk Thanks. I don't want to jump to conclusions, so I will add only my observations:

1. if I manually dispose merge operator at the and of FunctionalTests (before options destructor) everything is fine.
2. If I dispose merge operator inside options destructor merge operator destroy fails.

[mkosieradzki]

@warrenfalk - feel free to close all my issues/PRs. I have created my own based RocksDB C# wrapper from scratch using old good IDisposable, P/Invoke, dynamic linking and it using latest bits from rocksdb repo and it works perfectly.