CoreyKaylor / Lightning.NET

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

Exception on cursor.MoveToAndGet() #94

Closed JasonPunyon closed 4 years ago

JasonPunyon commented 7 years ago

Greetings and Salutations!

Love the library.

So, the following code throws an exception (MDB_BAD_VALSIZE) on the .MoveToAndGet(...) call.

using (var env = new LightningEnvironment(@"c:\test\"))
{
    env.Open();
    using (var tx = env.BeginTransaction())
    using (var db = tx.OpenDatabase())
    {
    var cursor = tx.CreateCursor(db);
    cursor.MoveToAndGet(new[] { (byte)123 });
    }
}

If the cursor operation couldn't be executed for some reason, I'd expect the call to just return false with no exception.

CoreyKaylor commented 7 years ago

Seeing as the title has changed, does that mean you are consistently getting getting an exception even when the database is not empty? Or is it only when empty?

mr-miles commented 7 years ago

I was debugging a similar issue and have a repro for this. Using latest nuget package (0.9.8).

I consistently get the exceptions when I go to read current in the below, when the database is empty.

        var path = @"C:\temp\tester\";
        using (var env = new LightningEnvironment(path))
        {
            env.Open();
            using (var txn = env.BeginTransaction())
            using (var db = txn.OpenDatabase(configuration: new DatabaseConfiguration {Flags = DatabaseOpenFlags.Create}))
            {
                // write a value
                using (var cur = txn.CreateCursor(db))
                {
                    var k = new byte[] { 200 };
                    // found is correctly false
                    var found = cur.MoveToAndGet(k);
                    // NullReference or Invalid argument exceptions occur for these
                    var fails2 = cur.Current;
                    var fails = cur.GetCurrent();
                }
            }
        }
        Directory.Delete(path, true);

If I add an item to the database I get exceptions from the .Current property, but not GetCurrent(). And if I call GetCurrent() first, then .Current works as expected:

        var path = @"C:\temp\tester\";
        using (var env = new LightningEnvironment(path))
        {
            env.Open();
            using (var txn = env.BeginTransaction())
            using (var db = txn.OpenDatabase(configuration: new DatabaseConfiguration {Flags = DatabaseOpenFlags.Create}))
            {
                // write a value
                using (var cur = txn.CreateCursor(db))
                {
                    cur.Put(new byte[] {35}, new byte[] {35}, CursorPutOptions.None);
                    var k = new byte[] { 200 };
                    // found is correctly false
                    var found = cur.MoveToAndGet(k);
                    // NullReference exception occur for these
                    var fails2 = cur.Current;
                    var fails = cur.GetCurrent();
                }
            }
        }
        Directory.Delete(path, true);
MaximGurschi commented 6 years ago

I think i have an explanation for why this happens. The difference comes from: MDB_SET vs MDB_SET_KEY. The former does not retrieve the key ptr from lmdb, the latter does.

When the .NET wrapper invokes the private method LightningCursor.Get that in turn calls Lmdb.mdb_cursor_get.

The latter creates a wrapper for the passed key via 'MarshalValueStructure' but it is immediately disposed! The Dispose frees up the GCHandle that the .NET wrapper allocates internally for the key that the caller passed in at the top of the stack.

Now how does this all fit together?

Since MDB_SET returns the key ptr to the MarshalValueStructure field, when you end up calling cursor.Current, this tries to 'unpack' (ValueStructure.GetBytes) the key and the value but the key is a freed ptr by this time! If however MDB_SET_KEY were to be used then the key would actually point to the lmdb key and not to 'MarshalValueStructure'.

So i think this is a bug in the .NET wrapper. The wrapper should not try to execute LightningCursor.Current if the key points to the MarshalValueStructure field.

But as @mr-miles pointed out the workaround is to: call MoveTo. If that returns true call GetCurrent (and then you are safe to call Current 0 or more times). This will work in all cases depending on if the key exists/does not exist.

Another way to do it in one go is to call MoveToAndGet and checking the result of that before looking up current.

CoreyKaylor commented 4 years ago

Based on the original report, I can't reproduce in the LightningDB.Tests project.

Regarding the further evidence, given that the original MoveToAndGet call returned false, I'm not entirely surprised that Current or GetCurrent() throws because it indicated it couldn't move to any key in the previous call.

On the last note on MDB_SET vs. MDB_SET_KEY. In this issue the MoveToAndGet actually uses SetKey, so I'm doubtful that it's as described. Not saying there's not an underlying problem, but I can't reproduce based on the initial description.

I'm sure this is long gone from everyone's minds given the fact that I've neglected the project for some time, but my circumstances have changed so feel free to reopen if you can reproduce this in a failed test.