broken transactional patterns

[phraktle]

the following pattern is broken:

    Transaction tx = env.createTransaction();
    try {
        return put(tx, key, value, flags);
    } finally {
        tx.commit();
    }

When put throws an error, the commit still runs. Which causes the commit itself to throw an error, masking the original problem.

[krisskross]

When does this happen? Could you provide the error codes for both put and commit?

[phraktle]

The put was throwing an "MDB_PAGE_FULL: Internal error - page has no more space" (the reasons are not yet obvious, but that's besides the point). However, the commit clobbered this with a "MDB_BAD_TXN: Transaction cannot recover - it must be aborted", so you couldn't see the underlying error.

[krisskross]

Is the commit automatically aborted by LMDB when this happens? Or must it be aborted manually to release the transaction?

[phraktle]

The transaction can only be aborted at this point. That's why the commit was throwing MDB_BAD_TXN.

[phraktle]

I'm not sure if there's a resource leak if you don't call abort explicitly, but there's certainly no harm in doing so :)

[krisskross]

Do you mind adding that to commit() in #24? I.e. call abort before throwing the exception?

[phraktle]

With the try-with-resources pattern, that's what actually happens: if the commit failed, the close performs an abort at the end of the block (since self wasn't zeroed by the commit). Doing an abort in commit is confusing.

Plus this pattern takes care of handling any potential suppressed exceptions.

[krisskross]

Yes, but what if you cannot use try-with-resources? Like when using ThreadLocal interceptors for example?

[phraktle]

Then the caller has to ensure closing the resource they have opened. Just like for any other resource, e.g. a file handle.

[krisskross]

I don't want to beat a dead horse but if the transaction knows that it cannot proceed why not abort right away? Just as a measure to prevent resource leaks. The user can still abort manually and this will be a no-op.

This a pretty abnormal situation so taking these extra measures might be appropriate?

[phraktle]

The reason I'm resisting is that I think it's confusing for the commit API call to implicitly also try an abort. And it also makes proper exception handling there nastier (catch potential the exception from commit, then catch potential exception from abort, add that as a suppressed exception to the first Throwable, etc) – while try-with-resources does all this automatically.

In general, most lmdbjni API users will either do atomic get/put calls, or if they do multiple operations in a transaction, they would use try-with-resources (which is much better than the pattern proposed by the current lmdbjni readme).

[krisskross]

Alright, let's settle on this approach.

[phraktle]

Thanks ;)