Skip to content

Adds Sqlite3.release/2 for prepared statements #155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 26, 2021
Merged

Conversation

warmwaffles
Copy link
Member

This should in theory fix #154

@warmwaffles warmwaffles requested a review from kevinlang August 11, 2021 13:26
@warmwaffles
Copy link
Member Author

Well that was weird, compilation segfaulted

@warmwaffles
Copy link
Member Author

I wonder if the segfault is from enif_release_resource(). When I run mix test on my local machine I do not get the segfault. I'll check if I can reproduce in a Docker container.

@warmwaffles
Copy link
Member Author

For anyone following along, I haven't forgotten about this. Just been busy and haven't been able to get back to this.

The garbage collector will come back through later and release the
opaque nif resource. This will at least reclaim a large portion of the
memory taken up by the prepared statement.
@warmwaffles
Copy link
Member Author

@kevinlang I figured out where the seg fault was coming from. I over released a resource. So once you call enif_make_resource that gives you an opaque pointer to fiddle with and then call enif_release_resource after to decrement the reference counter and hand it back to erlang to manage.

If you already released the resource and then later try to double release it like this

static ERL_NIF_TERM
exqlite_release(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
{
    assert(env);

    statement_t* statement = NULL;
    connection_t* conn     = NULL;

    if (argc != 2) {
        return enif_make_badarg(env);
    }

    if (!enif_get_resource(env, argv[0], connection_type, (void**)&conn)) {
        return make_error_tuple(env, "invalid_connection");
    }

    if (!enif_get_resource(env, argv[1], statement_type, (void**)&statement)) {
        return make_error_tuple(env, "invalid_statement");
    }

    if (statement->statement) {
        sqlite3_finalize(statement->statement);
        statement->statement = NULL;
    }
    //
    // KABOOM
    //
    enif_release_resource(statement);

    return make_atom(env, "ok");
}

Previously when the statement was initialized and ready to go we immediately called release resource once we had the opaque handle to return.

    rc = sqlite3_prepare_v3(conn->db, (char*)bin.data, bin.size, 0, &statement->statement, NULL);
    if (rc != SQLITE_OK) {
        enif_release_resource(statement);
        return make_sqlite3_error_tuple(env, rc, conn->db);
    }

    result = enif_make_resource(env, statement);
    enif_release_resource(statement);

@warmwaffles
Copy link
Member Author

I still don't know of a good way to test for memory leaks.

One thing I think we probably need to explore is a process terminating unexpectedly, and seeing if that leaks memory.

@warmwaffles warmwaffles linked an issue Aug 26, 2021 that may be closed by this pull request
@warmwaffles warmwaffles added this to the 1.0 milestone Aug 26, 2021
@kevinlang
Copy link
Member

Looks good! I also cannot think of a good way to test for memory leaks.

@warmwaffles
Copy link
Member Author

The only thing I could think of is making a C test harness and just having it run valgrind checking for leaks, but that's pandora's box.

@warmwaffles warmwaffles merged commit e13f712 into main Aug 26, 2021
@warmwaffles warmwaffles deleted the release-statement branch August 26, 2021 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically close prepared statements after execute Performance issues / possible memory leak
2 participants