Use Task System to schedule Conquest tasks

[xkoredev]

I affirm:

• I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
• I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
• I have read and understood the Contributing Guide and the Code of Conduct.
• I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

• Conquest system no longer running two separate instances
• Use task system to schedule conquest system tasks

Steps to test these changes

Run conquest updates. Make sure conquest map is consistent. Check all zones and make sure there are no crashes.

[zach2good]

I don't think task_system is going to work for your use case here if you have to submit a task to configure it - I think it's too simple of a building block to achieve what you want.

[zach2good]

What might work here, is if you make IMessageHandler also inherit from task_system, giving you access to the guts and the construction of everything. This would allow you to lazily set up the sql handle upon submitting the first task, or on construction, or whatever.

[zach2good]

and just because this was the review point that made me remember the term I've been trying to remember for WEEKS: https://en.wikipedia.org/wiki/Convention_over_configuration

[xkoredev]

So been thinking about this. I wouldn't need to do this if it wasn't for the warning that pops up when checking if sql query is executed in the same thread it's instantiated.

Because all queries are going to happen in the same thread (the task system thread), it's okay that we instantiate sql in a different thread. Ideally, the check would only happen if two queries are called from different threads. Would you be okay with that change?

[xkoredev]

Ah I do see that sqlconnection fires a query on instantiation, which makes this change harder

[zach2good]

Absolutely not. I don't play up my 'trap card' to be cute or funny, I do it because it's a legitimate problem that was very annoying to diagnose and fix.

server/src/common/sql.h

Line 30 in b61c149

// NOTE: Access to any of the MySQL resources is NOT thread-safe.

[zach2good]

The warning triggers in the exact scenario that results in UB, most often seen as the sql handle getting wiped out - so don't ignore it

[zach2good]

std::make_unique

[zach2good]

But as I mentioned above, I'd look into making this class: class IMessageHandler : public ts::task_system

[zach2good]

You could also make it inherit from SqlConnection 👀

[zach2good]

If you use smart pointers, there's no need for this

[xkoredev]

will use a smart pointer. I was following the Async example. Thought there was a reason for not using a smart pointer

[zach2good]

You can also edit Async to look like this:

#pragma once

#include "sql.h"
#include <task_system.hpp>

#include <functional>
#include <string>

class Async : public ts::task_system
{
public:
    ~Async() = default;

    static Async* getInstance();

    void query(std::string const& query);
    void query(std::function<void(SqlConnection*)> const& func);
    void submit(std::function<void()> const& func);

private:
    Async();
};

#include "async.h"

#include "sql.h"
#include <task_system.hpp>

namespace
{
    // Allocated on and owned the main thread
    std::unique_ptr<Async> _instance = nullptr;

    // Allocated on and owned by the task_system worker thread
    std::unique_ptr<SqlConnection> _sql = nullptr;
} // namespace

Async::Async()
: ts::task_system(1)
{
}

Async* Async::getInstance()
{
    if (_instance == nullptr)
    {
        _instance = std::make_unique<Async>();
    }
    return _instance.get();
}

void Async::query(std::string const& query)
{
    // clang-format off
    this->schedule([query]()
    {
        TracySetThreadName("Async Worker Thread");
        if (_sql == nullptr)
        {
            _sql = std::make_unique<SqlConnection>();
        }
        _sql->Query(query.c_str());
    });
    // clang-format on
}

// NOTE: Be _very_ careful when defining your sql argument in the passed-in function.
//     : If you define your arg as _sql, but then call sql, it will use the global
//     : SQLConnection, which is on the main thread.
//     : Remember that SQLConnection is NOT THREAD-SAFE!
void Async::query(std::function<void(SqlConnection*)> const& func)
{
    // clang-format off
    this->schedule([func]()
    {
        TracySetThreadName("Async Worker Thread");
        if (_sql == nullptr)
        {
            _sql = std::make_unique<SqlConnection>();
        }
        func(_sql.get());
    });
    // clang-format on
}

void Async::submit(std::function<void()> const& func)
{
    // clang-format off
    this->schedule([func]()
    {
        TracySetThreadName("Async Worker Thread");
        func();
    });
    // clang-format on
}

And then use Async::getInstance()->query()/submit(), then you won't have to deal with any of this crap on your own. Only downside would be that all systems that call it share the same work queue, but I think that'll be OK?

[zach2good]

But you'd want to make ts::task_system inheritance private, so you don't leak it to consumers

[xkoredev]

But you'd want to make ts::task_system inheritance private, so you don't leak it to consumers

Think ill use the modified async version. The only downside is that single thread for all systems, but in theory that is okay with our current usage. If it does become a problem, couldn't we just create non singleton versions of Async per system?

[xkoredev]

But you'd want to make ts::task_system inheritance private, so you don't leak it to consumers

Think ill use the modified async version. The only downside is that single thread for all systems, but in theory that is okay with our current usage. If it does become a problem, couldn't we just create non singleton versions of Async per system?

hmm I do see that currently chat audits go through this thread. It may be a better idea to have separate Async instances per system

[zach2good]

Chat audits are on the map server and this targets world server no?

[xkoredev]

Chat audits are on the map server and this targets world server no?

oooh good point. We can do this then.

[zach2good]

I don't think it's worth worrying too much about performance until it becomes an actual problem, everything is lock less and on a different thread, this will hold the workload easily

[xkoredev]

You can also edit Async to look like this:

#pragma once

#include "sql.h"
#include <task_system.hpp>

#include <functional>
#include <string>

class Async : public ts::task_system
{
public:
    ~Async() = default;

    static Async* getInstance();

    void query(std::string const& query);
    void query(std::function<void(SqlConnection*)> const& func);
    void submit(std::function<void()> const& func);

private:
    Async();
};

#include "async.h"

#include "sql.h"
#include <task_system.hpp>

namespace
{
    // Allocated on and owned the main thread
    std::unique_ptr<Async> _instance = nullptr;

    // Allocated on and owned by the task_system worker thread
    std::unique_ptr<SqlConnection> _sql = nullptr;
} // namespace

Async::Async()
: ts::task_system(1)
{
}

Async* Async::getInstance()
{
    if (_instance == nullptr)
    {
        _instance = std::make_unique<Async>();
    }
    return _instance.get();
}

void Async::query(std::string const& query)
{
    // clang-format off
    this->schedule([query]()
    {
        TracySetThreadName("Async Worker Thread");
        if (_sql == nullptr)
        {
            _sql = std::make_unique<SqlConnection>();
        }
        _sql->Query(query.c_str());
    });
    // clang-format on
}

// NOTE: Be _very_ careful when defining your sql argument in the passed-in function.
//     : If you define your arg as _sql, but then call sql, it will use the global
//     : SQLConnection, which is on the main thread.
//     : Remember that SQLConnection is NOT THREAD-SAFE!
void Async::query(std::function<void(SqlConnection*)> const& func)
{
    // clang-format off
    this->schedule([func]()
    {
        TracySetThreadName("Async Worker Thread");
        if (_sql == nullptr)
        {
            _sql = std::make_unique<SqlConnection>();
        }
        func(_sql.get());
    });
    // clang-format on
}

void Async::submit(std::function<void()> const& func)
{
    // clang-format off
    this->schedule([func]()
    {
        TracySetThreadName("Async Worker Thread");
        func();
    });
    // clang-format on
}

And then use Async::getInstance()->query()/submit(), then you won't have to deal with any of this crap on your own. Only downside would be that all systems that call it share the same work queue, but I think that'll be OK?

Compiler is not liking the private Async ctor when calling: std::make_unique(). Make this is why it wasn't a unique ptr before?

[zach2good]

Use the regular boilerplate for a singleton for Async, now that we're going to use it as a singleton:

#include <iostream>
#include <string>
#include <memory>

// async.h
class Async
{
public:
    static Async* getInstance();

    // other methods here

private:
    Async() = default;
    ~Async() = default;

    Async(const Async&) = delete;
    Async& operator=(const Async&) = delete;
};

// async.cpp
Async* Async::getInstance()
{
    static Async instance;
    return &instance;
}

int main()
{
    return 0;
}

[xkoredev]

Use the regular boilerplate for a singleton for Async, now that we're going to use it as a singleton:

#include <iostream>
#include <string>
#include <memory>

// async.h
class Async
{
public:
    static Async* getInstance();

    // other methods here

private:
    Async() = default;
    ~Async() = default;

    Async(const Async&) = delete;
    Async& operator=(const Async&) = delete;
};

// async.cpp
Async* Async::getInstance()
{
    static Async instance;
    return &instance;
}

int main()
{
    return 0;
}

oh... I don't know why I was using std pointer ... mb

[xkoredev]

You can also edit Async to look like this:

#pragma once

#include "sql.h"
#include <task_system.hpp>

#include <functional>
#include <string>

class Async : public ts::task_system
{
public:
    ~Async() = default;

    static Async* getInstance();

    void query(std::string const& query);
    void query(std::function<void(SqlConnection*)> const& func);
    void submit(std::function<void()> const& func);

private:
    Async();
};

#include "async.h"

#include "sql.h"
#include <task_system.hpp>

namespace
{
    // Allocated on and owned the main thread
    std::unique_ptr<Async> _instance = nullptr;

    // Allocated on and owned by the task_system worker thread
    std::unique_ptr<SqlConnection> _sql = nullptr;
} // namespace

Async::Async()
: ts::task_system(1)
{
}

Async* Async::getInstance()
{
    if (_instance == nullptr)
    {
        _instance = std::make_unique<Async>();
    }
    return _instance.get();
}

void Async::query(std::string const& query)
{
    // clang-format off
    this->schedule([query]()
    {
        TracySetThreadName("Async Worker Thread");
        if (_sql == nullptr)
        {
            _sql = std::make_unique<SqlConnection>();
        }
        _sql->Query(query.c_str());
    });
    // clang-format on
}

// NOTE: Be _very_ careful when defining your sql argument in the passed-in function.
//     : If you define your arg as _sql, but then call sql, it will use the global
//     : SQLConnection, which is on the main thread.
//     : Remember that SQLConnection is NOT THREAD-SAFE!
void Async::query(std::function<void(SqlConnection*)> const& func)
{
    // clang-format off
    this->schedule([func]()
    {
        TracySetThreadName("Async Worker Thread");
        if (_sql == nullptr)
        {
            _sql = std::make_unique<SqlConnection>();
        }
        func(_sql.get());
    });
    // clang-format on
}

void Async::submit(std::function<void()> const& func)
{
    // clang-format off
    this->schedule([func]()
    {
        TracySetThreadName("Async Worker Thread");
        func();
    });
    // clang-format on
}

And then use Async::getInstance()->query()/submit(), then you won't have to deal with any of this crap on your own. Only downside would be that all systems that call it share the same work queue, but I think that'll be OK?

I'm not sure we can just rely on ->query() to run queries. How would you then do something like
sendInfluencesMsg() where we obtain data from DB (we would need to fetch data, so query doesn't work as it only updates data) in the same queue as we would perform other operations like updateVanaWeekly.

[zach2good]

I've done the refactoring to make Async a singleton, and it'll be scalable to multiple threads if the workload ever demands it:
#4628

[zach2good]

I'm not sure we can just rely on ->query() to run queries. How would you then do something like
sendInfluencesMsg() where we obtain data from DB (we would need to fetch data, so query doesn't work as it only updates data) in the same queue as we would perform other operations like updateVanaWeekly.

At this point, just abandon multi-threading and revert everything to lockstep ticks on the world server - being fed messages from the ZMQ queue, and pushing messages back onto the ZMQ queue. Once besieged is "done" we can come back and look at whether or not it's worth breaking things out onto individual threads.

[zach2good]

Use this PR to fix this: Conquest system no longer running two separate instances

If that requires mutexes, so be it. I'll vet it as it happens

[zach2good]

I've done a different version of this refactoring here: #4632

This should be sufficient for allowing you to fix those other conquest bugs (if this was needed for that), and for your work on besieged. If things need to be re-threaded later, we'll deal with it later - but I think it's unlikely.

[xkoredev]

I've done a different version of this refactoring here: #4632

This should be sufficient for allowing you to fix those other conquest bugs (if this was needed for that), and for your work on besieged. If things need to be re-threaded later, we'll deal with it later - but I think it's unlikely.

works for me. will integrate it to ASB once merged