Skip to content

Commit

Permalink
🧼added TSan to check data race problem
Browse files Browse the repository at this point in the history
🐛fixed data race in Task, and some refactor
⚒️changed to sigaction instead of signal
⛏️updated app settings for demo
  • Loading branch information
Lord-Turmoil committed Nov 27, 2024
1 parent accb2ff commit 36db4ac
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 84 deletions.
3 changes: 3 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ add_subdirectory(libs)

if (CMAKE_BUILD_TYPE STREQUAL "Debug")
add_compile_definitions(DEBUG)
# ASan and TSan are incompatible, so we can't use them together.
# And because we use std::shared_ptr, it is most likely that there is no need for ASan.
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=undefined,thread")
endif()

# strict warnings
Expand Down
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
> [!NOTE]
> Before you move on, ensure you have CMake and your C++ compiler supports C++ 17.🫡
> [!WARNING]
> If you build **minet-core** with Debug configuration on Ubuntu 20.04, you may get linker error saying missing `libtsan_preinit.o`. Check [(TSAN) /usr/bin/ld can't find libtsan_preinit.o](https://stackoverflow.com/questions/77858687/tsan-usr-bin-ld-cant-find-libtsan-preinit-o) for solutions.
## Prepare the Repository

**minet-core** relies on some third-party libraries, you can clone it with the following command.
Expand Down
2 changes: 1 addition & 1 deletion demo/appsettings.jsonc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"loggers": {
"Demo": {
"level": "Debug",
"pattern": "%^[%Y-%m-%d %H:%M:%S] %l [%6n]: %v%$",
"pattern": "[%Y-%m-%d %H:%M:%S] %^%l%$ [%6n]: %v",
"sinks": [
{
"file": "stdout"
Expand Down
8 changes: 5 additions & 3 deletions scripts/demo.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,24 @@
# This script is used to run the minet demo.

function run_server() {
PROFILE=debug
# Build the project
echo -e "\033[0;33mBuilding project...\033[0m"
./scripts/build.sh release >/dev/null
./scripts/build.sh $PROFILE >/dev/null
if [ $? -ne 0 ]; then
echo "Failed to build the project"
exit 1
fi

BIN=./build-release/demo/minet-demo
BIN=./build-$PROFILE/demo/minet-demo
ARGS=${1:-"demo/appsettings.jsonc"}
if [ ! -f "$BIN" ]; then
echo "Binary file not found: $BIN"
exit 1
fi
echo -e "\033[0;36m$BIN $ARGS\033[0m"
$BIN $ARGS
# do not check for signal-unsafe call, as it is a desired behavior
TSAN_OPTIONS="report_signal_unsafe=0" $BIN $ARGS
}

function run_client() {
Expand Down
200 changes: 132 additions & 68 deletions src/threading/Task.h
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
/**
* @author Tony S.
* @details Task wrapper.
* @note
* Simply using std::future with std::async may cause data races.
* So, we take advantage of std::packaged_task to wrap the task.
*/

#pragma once

#include "minet/common/Assert.h"

#include <functional>
#include <future>
#include <utility>
#include <utility> // std::forward

MINET_BEGIN

Expand All @@ -26,40 +27,22 @@ class Task final : public std::enable_shared_from_this<Task>
public:
using TaskFn = std::function<void()>;

explicit Task(TaskFn routine, Private) : _routine(std::move(routine))
{
}
Task(TaskFn routine, Private);

static Ref<Task> Create(const TaskFn& routine)
{
return CreateRef<Task>(routine, Private());
}
Task(const Task&) = delete;
Task& operator=(const Task&) = delete;
Task(Task&& other) noexcept;
Task& operator=(Task&& other) noexcept;

static Ref<Task> Completed()
{
return CreateRef<Task>([] {}, Private());
}
template <typename TRoutine> static Ref<Task> Create(TRoutine&& routine);
static Ref<Task> Completed();

Ref<Task> StartAsync()
{
MINET_ASSERT(!_future.valid()); // not started before
_future = std::async(std::launch::async, _routine);
return shared_from_this();
}

void Await()
{
MINET_ASSERT(_future.valid()); // not started before
_future.get();
}

void Run()
{
StartAsync()->Await();
}
Ref<Task> StartAsync();
void Await();
void Run();

private:
TaskFn _routine;
std::packaged_task<void()> _task;
std::future<void> _future;
};

Expand All @@ -76,55 +59,136 @@ template <typename TResult> class ValueTask final : public std::enable_shared_fr
public:
using ValueTaskFn = std::function<TResult()>;

explicit ValueTask(ValueTaskFn routine, Private) : _routine(std::move(routine))
{
}
ValueTask(ValueTaskFn routine, Private);

/**
* @brief Create a task with the given thread.
*/
static Ref<ValueTask> Create(ValueTaskFn routine)
{
return CreateRef<Task>(std::move(routine), Private());
}
ValueTask(const ValueTask& other) = delete;
ValueTask& operator=(const ValueTask& other) = delete;
ValueTask(ValueTask&& other) noexcept;
ValueTask& operator=(ValueTask&& other) noexcept;

template <typename TRoutine> static Ref<ValueTask> Create(TRoutine&& routine);
static Ref<ValueTask> Completed(const TResult& result);

Ref<ValueTask> StartAsync();
TResult Await();
TResult Run();

private:
std::packaged_task<TResult()> _task;
std::future<TResult> _future;
};

/*
* ===================================================================
* ---------------------- Task Implementation ------------------------
* ===================================================================
*/
inline Task::Task(TaskFn routine, Private) : _task(std::move(routine))
{
}

inline Task::Task(Task&& other) noexcept : _task(std::move(other._task)), _future(std::move(other._future))
{
}

/**
* @brief Create a completed task from result.
*/
static Ref<ValueTask> Completed(const TResult& result)
inline Task& Task::operator=(Task&& other) noexcept
{
if (this != &other)
{
return CreateRef<Task>([result]() -> TResult { return result; }, Private());
_task = std::move(other._task);
_future = std::move(other._future);
}
return *this;
}

template <typename TRoutine> Ref<Task> Task::Create(TRoutine&& routine)
{
return CreateRef<Task>(std::forward<TRoutine>(routine), Private());
}

inline Ref<Task> Task::Completed()
{
return CreateRef<Task>([] {}, Private());
}

inline Ref<Task> Task::StartAsync()
{
_future = _task.get_future();
_task();
return shared_from_this();
}

/**
* @brief Start the task asynchronously.
* @return The task itself.
*/
Ref<ValueTask> StartAsync()
inline void Task::Await()
{
if (!_future.valid())
{
MINET_ASSERT(!_future.valid()); // not started before
_future = std::async(std::launch::async, _routine);
return this->shared_from_this();
throw std::runtime_error("Task not started");
}
_future.get();
}

inline void Task::Run()
{
StartAsync()->Await();
}

/*
* ===================================================================
* --------------------- ValueTask Implementation --------------------
* ===================================================================
*/

template <typename TResult> ValueTask<TResult>::ValueTask(ValueTaskFn routine, Private) : _task(std::move(routine))
{
}

/**
* @brief Wait for the task to finish.
* @return Result of the task.
*/
TResult Await()
template <typename TResult>
ValueTask<TResult>::ValueTask(ValueTask&& other) noexcept
: _task(std::move(other._task)), _future(std::move(other._future))
{
}

template <typename TResult> ValueTask<TResult>& ValueTask<TResult>::operator=(ValueTask&& other) noexcept
{
if (this != &other)
{
MINET_ASSERT(_future.valid()); // not started before
return _future.get();
_task = std::move(other._task);
_future = std::move(other._future);
}
return *this;
}

template <typename TResult>
template <typename TRoutine>
Ref<ValueTask<TResult>> ValueTask<TResult>::Create(TRoutine&& routine)
{
return CreateRef<ValueTask>(std::forward<TRoutine>(routine), Private());
}

template <typename TResult> Ref<ValueTask<TResult>> ValueTask<TResult>::Completed(const TResult& result)
{
return CreateRef<ValueTask>([result] { return result; }, Private());
}

TResult Run()
template <typename TResult> Ref<ValueTask<TResult>> ValueTask<TResult>::StartAsync()
{
_future = _task.get_future();
_task();
return this->shared_from_this();
}

template <typename TResult> TResult ValueTask<TResult>::Await()
{
if (!_future.valid())
{
return StartAsync().Await();
throw std::runtime_error("Task not started");
}
return _future.get();
}

private:
ValueTaskFn _routine;
std::future<TResult> _future;
};
template <typename TResult> TResult ValueTask<TResult>::Run()
{
return StartAsync()->Await();
}

MINET_END
29 changes: 17 additions & 12 deletions src/utils/Native.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,31 @@ static void _SignalHandler(int sig);

int SetSignalHandler(int sig, const std::function<void()>& handler, bool once)
{
sSignalHandlers[sig] = { sig, once, handler };
if (auto r = signal(sig, _SignalHandler); r == SIG_ERR)
struct sigaction act;
act.sa_handler = _SignalHandler;
sigemptyset(&act.sa_mask);
act.sa_flags = SA_SIGINFO;
if (once)
{
return -1;
act.sa_flags |= SA_RESETHAND;
}
MINET_TRY(sigaction(sig, &act, nullptr));

sSignalHandlers[sig] = { sig, once, handler };

return 0;
}

int RemoveSignalHandler(int sig)
{
sSignalHandlers.erase(sig);
if (auto r = signal(sig, SIG_DFL); r == SIG_ERR)
{
return -1;
}

struct sigaction act;
act.sa_handler = SIG_DFL;
sigemptyset(&act.sa_mask);
act.sa_flags = SA_SIGINFO;
MINET_TRY(sigaction(sig, &act, nullptr));

return 0;
}

Expand All @@ -51,11 +61,6 @@ void _SignalHandler(int sig)
{
sSignalHandlers.erase(it);
}
else
{
// FIXME: Error omitted.
signal(sig, _SignalHandler);
}
}

} // namespace native
Expand Down

0 comments on commit 36db4ac

Please sign in to comment.