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
⚒️changed to sigaction instead of signal
⛏️updated app settings for demo
  • Loading branch information
Lord-Turmoil committed Nov 27, 2024
1 parent accb2ff commit f4189fb
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 29 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
37 changes: 20 additions & 17 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,13 +27,13 @@ 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))
explicit Task(TaskFn routine, Private) : _task(std::move(routine))
{
}

static Ref<Task> Create(const TaskFn& routine)
static Ref<Task> Create(TaskFn&& routine)
{
return CreateRef<Task>(routine, Private());
return CreateRef<Task>(std::forward<TaskFn>(routine), Private());
}

static Ref<Task> Completed()
Expand All @@ -42,14 +43,13 @@ class Task final : public std::enable_shared_from_this<Task>

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

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

Expand All @@ -59,7 +59,7 @@ class Task final : public std::enable_shared_from_this<Task>
}

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

Expand All @@ -76,7 +76,7 @@ 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))
explicit ValueTask(ValueTaskFn routine, Private) : _task(std::move(routine))
{
}

Expand All @@ -85,7 +85,7 @@ template <typename TResult> class ValueTask final : public std::enable_shared_fr
*/
static Ref<ValueTask> Create(ValueTaskFn routine)
{
return CreateRef<Task>(std::move(routine), Private());
return CreateRef<Task>(std::forward<ValueTaskFn>(routine), Private());
}

/**
Expand All @@ -102,8 +102,8 @@ template <typename TResult> class ValueTask final : public std::enable_shared_fr
*/
Ref<ValueTask> StartAsync()
{
MINET_ASSERT(!_future.valid()); // not started before
_future = std::async(std::launch::async, _routine);
_future = _task.get_future();
_task();
return this->shared_from_this();
}

Expand All @@ -113,8 +113,11 @@ template <typename TResult> class ValueTask final : public std::enable_shared_fr
*/
TResult Await()
{
MINET_ASSERT(_future.valid()); // not started before
return _future.get();
if (!_future.valid())
{
throw std::runtime_error("Task not started");
}
_future.get();
}

TResult Run()
Expand All @@ -123,7 +126,7 @@ template <typename TResult> class ValueTask final : public std::enable_shared_fr
}

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

Expand Down
22 changes: 14 additions & 8 deletions src/utils/Native.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,27 @@ static void _SignalHandler(int sig);

int SetSignalHandler(int sig, const std::function<void()>& handler, bool once)
{
struct sigaction act;
act.sa_handler = _SignalHandler;
sigemptyset(&act.sa_mask);
act.sa_flags = SA_SIGINFO;
MINET_TRY(sigaction(sig, &act, nullptr));

sSignalHandlers[sig] = { sig, once, handler };
if (auto r = signal(sig, _SignalHandler); r == SIG_ERR)
{
return -1;
}

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 Down

0 comments on commit f4189fb

Please sign in to comment.