[Bug]: db.executeQuery: Commands out of sync; you can't run this command now

By submitting this bug issue, you agree to the following.

[X] This is a bug in the software that resides in this repository, and not a support matter (use https://otland.net/forums/support.16/ for support)
[X] This issue is reproducible without changes to the C++ code in this repository
[X] This bug has not been resolved in master branch
[X] There is no existing issue for this bug already

Does this bug crash tfs?

Server Version

1.7 (Master), 1.6 (Release)

Operation System

all (listed below)

OS Description

No response

Bug description

After running SELECT query with Database::executeQuery (Lua: db.query) server is not able to execute SQL queries anymore.

Ex. this C++ code executes executeQuery which may return some results (if anything is optimized): if (db.executeQuery(fmt::format("OPTIMIZE TABLE{:s}", tableName))) {

Possible Pull Requests which are to blame

4626

Steps to reproduce

Run db.query (you should run db.storeQuery) with SQL query that returns some result from MySQL
OTS is not able to run any other SQL query (save player, load player etc.). Every query shows error: Commands out of sync; you can't run this command now

Code to reproduce the issue:

-- ex. as talkaction
local talk = TalkAction("!crashsqlconnection")

function talk.onSay(player, words, param)
    db.query("SELECT 1")
    return true
end

talk:register()

Actual Behavior

Running Database::executeQuery with SELECT blocks all next SQL queries.

Expected Behavior

Small bug in C++/Lua should not block all next SQL queries.

Backtrace

No response

I fixed this by replacing:

bool Database::executeQuery(const std::string& query)
{
    std::lock_guard<std::recursive_mutex> lockGuard(databaseLock);
    return ::executeQuery(handle, query, retryQueries);
}

with:

bool Database::executeQuery(const std::string& query)
{
    std::lock_guard<std::recursive_mutex> lockGuard(databaseLock);
    auto success = ::executeQuery(handle, query, retryQueries);
    // we should call that every time as someone would call executeQuery('SELECT...')
    // as it is described in MySQL manual: "it doesn't hurt" :P
    tfs::detail::MysqlResult_ptr res{mysql_store_result(handle.get())};

    return success;
}

Can anyone experienced with shared pointers check, if there is no memory leak?

From my understanding of shared pointers: It will convert result (if exists) into shared pointer and this shared pointer will be deleted at end of function. Deletion of MysqlResult_ptr will call mysql_free_result ( https://github.com/otland/forgottenserver/blob/1.6/src/database.h#L14-L21 ), so it should not leak any memory.

otland / forgottenserver