ThreadPool: Fix check-then-act race condition

Author: MorganCaron

modules/Chrono/Chronometer.mpp

@@ -32,7 +32,7 @@ export namespace CppUtils::Chrono
 			str += " " + std::to_string(microsec.count()) + "us";
 		if (ns.count() > 0)
 			str += " " + std::to_string(ns.count()) + "ns";
-		return !str.empty() ? str.substr(1) : "0ns";
+		return not std::empty(str) ? str.substr(1) : "0ns";
 	}
 
 	template<Clock Clock = std::chrono::high_resolution_clock>

modules/Container/Sequence.mpp

@@ -16,9 +16,9 @@ export namespace CppUtils::Container::Sequence
 	[[nodiscard]] inline auto getDifferences(std::span<const T> list0, std::span<const T> list1) -> std::vector<Difference>
 	{
 		auto differences = std::vector<Difference>{};
-		differences.reserve(std::max(list0.size(), list1.size()));
+		differences.reserve(std::max(std::size(list0), std::size(list1)));
 		auto i = 0uz, j = 0uz, diffLength = 0uz;
-		while (i < list0.size() and j < list1.size())
+		while (i < std::size(list0) and j < std::size(list1))
 		{
 			const auto& lhs = list0[i];
 			const auto& rhs = list1[j];
@@ -52,10 +52,10 @@ export namespace CppUtils::Container::Sequence
 			++i;
 			++j;
 		}
-		auto offsetList0 = list0.size() - i;
+		auto offsetList0 = std::size(list0) - i;
 		while (offsetList0--)
 			differences.emplace_back(Difference::Removed);
-		auto offsetList1 = list1.size() - j;
+		auto offsetList1 = std::size(list1) - j;
 		while (offsetList1--)
 			differences.emplace_back(Difference::Added);
 		return differences;

modules/Network/Client.mpp

@@ -25,7 +25,7 @@ export namespace CppUtils::Network
 
 		inline auto connect(std::string_view ip, std::uint16_t port) const -> void
 		{
-			auto address = makeAddress(ip.empty() ? "127.0.0.1" : ip, port);
+			auto address = makeAddress(std::empty(ip) ? "127.0.0.1" : ip, port);
 			if (address.length == 0)
 				throw std::runtime_error{"CppUtils::Network::Client::connect: Invalid address length"};
 #if defined(OS_WINDOWS)

modules/Network/Socket.mpp

@@ -305,7 +305,7 @@ export namespace CppUtils::Network
 				ipv4->sin_family = AF_INET;
 				ipv4->sin_port = Math::toBigEndian(port);
 
-				if (ip.empty())
+				if (std::empty(ip))
 					ipv4->sin_addr.s_addr = INADDR_ANY;
 				else if (::inet_pton(AF_INET, std::data(ip), std::addressof(ipv4->sin_addr)) <= 0)
 					throw std::invalid_argument{"CppUtils::Network::Socket::makeAddress: Invalid IPv4 address"};
@@ -318,7 +318,7 @@ export namespace CppUtils::Network
 				ipv6->sin6_family = AF_INET6;
 				ipv6->sin6_port = Math::toBigEndian(port);
 
-				if (ip.empty())
+				if (std::empty(ip))
 					ipv6->sin6_addr = in6addr_any;
 				else if (::inet_pton(AF_INET6, std::data(ip), std::addressof(ipv6->sin6_addr)) <= 0)
 					throw std::invalid_argument{"CppUtils::Network::Socket::makeAddress: Invalid IPv6 address"};

modules/String/Hash.mpp

@@ -29,7 +29,7 @@ export namespace CppUtils::String
 		inline constexpr Hasher(const CharT (&cString)[N]):
 			hash{CppUtils::String::hash(cString)}
 		{
-			std::copy(cString, cString + N, name.begin());
+			std::copy(cString, cString + N, std::begin(name));
 		}
 
 		[[nodiscard]] inline constexpr operator Hash() const noexcept

modules/String/Utility.mpp

@@ -78,14 +78,14 @@ export namespace CppUtils::String
 		[[nodiscard]] inline auto concatenateStringsWithSeparator(const std::vector<std::string>& strings, std::string_view separator) -> std::string
 		{
 			return std::accumulate(std::cbegin(strings), std::cend(strings), std::string{}, [separator](const std::string& lhs, const std::string& rhs) {
-				return lhs.empty() ? rhs : (lhs + std::data(separator) + rhs);
+				return std::empty(lhs) ? rhs : (lhs + std::data(separator) + rhs);
 			});
 		}
 
 		[[nodiscard]] inline auto concatenateStringsWithSeparator(const std::vector<std::string_view>& strings, std::string_view separator) -> std::string
 		{
 			return std::accumulate(std::cbegin(strings), std::cend(strings), std::string{}, [separator](std::string_view lhs, std::string_view rhs) {
-				return lhs.empty() ? std::string{rhs} : (std::string{lhs} + std::string{separator} + std::string{rhs});
+				return std::empty(lhs) ? std::string{rhs} : (std::string{lhs} + std::string{separator} + std::string{rhs});
 			});
 		}
 	}

modules/Thread/ThreadLoop.mpp

@@ -109,9 +109,6 @@ export namespace CppUtils::Thread
 
 		inline auto isStopRequested() const -> bool
 		{
-			auto lockGuard = std::unique_lock{m_mutex};
-			if (not m_thread.joinable())
-				return false;
 			return m_thread.get_stop_token().stop_requested();
 		}
 
@@ -127,14 +124,17 @@ export namespace CppUtils::Thread
 		{
 			try
 			{
-				auto lockGuard = std::unique_lock{m_mutex};
-				if (not m_thread.joinable() or m_thread.get_stop_token().stop_requested())
-					return;
-				m_thread.request_stop();
-				if (m_interruptFunction)
+				auto interruptFunction = std::function<void()>{};
+				{
+					auto lockGuard = std::unique_lock{m_mutex};
+					if (not m_thread.joinable() or m_thread.get_stop_token().stop_requested())
+						return;
+					m_thread.request_stop();
+					interruptFunction = m_interruptFunction;
+				}
+
+				if (interruptFunction)
 				{
-					auto interruptFunction = m_interruptFunction;
-					lockGuard.unlock();
 					try
 					{
 						interruptFunction();
@@ -143,7 +143,6 @@ export namespace CppUtils::Thread
 					{
 						Logger::print<"error">("ThreadLoop: interruptFunction threw during stop");
 					}
-					lockGuard.lock();
 				}
 				if (m_thread.joinable())
 					m_thread.join();

modules/Thread/ThreadPool.mpp

@@ -107,16 +107,17 @@ export namespace CppUtils::Thread
 
 				task = std::move(tasksQueueAccessor->front());
 				tasksQueueAccessor->pop();
+				m_activeWorkers.fetch_add(1, std::memory_order_relaxed);
 			}
 
-			m_activeWorkers.fetch_add(1, std::memory_order_relaxed);
 			task();
 			m_activeWorkers.fetch_sub(1, std::memory_order_relaxed);
 
-			if (m_activeWorkers.load(std::memory_order_relaxed) == 0 and isTasksQueueEmpty())
+			if (m_activeWorkers.load(std::memory_order_relaxed) == 0)
 			{
 				auto lockGuard = std::lock_guard{m_waitingMutex};
-				m_waitUntilFinishedCondition.notify_all();
+				if (isTasksQueueEmpty())
+					m_waitUntilFinishedCondition.notify_all();
 			}
 		}
 

tests/Container/Sequence.mpp

@@ -8,7 +8,7 @@ namespace
 	[[nodiscard]] auto generateDifferenceVector(std::string_view string) -> std::vector<CppUtils::Container::Sequence::Difference>
 	{
 		auto result = std::vector<CppUtils::Container::Sequence::Difference>{};
-		result.reserve(string.size());
+		result.reserve(std::size(string));
 		for (const auto c : string)
 		{
 			switch (c)