Improve work_queue thread pool: correctness fixes, performance, and tests

[Copilot]

Audit of the work_queue thread pool in src/YgorThreadPool.h for correctness and performance.

Correctness

• Catch all exceptions in workers — Only std::exception was caught; a non-std throw (e.g. throw 42;) would invoke std::terminate and crash the process. Added catch(...).
• Guard if(task) validity check with if constexpr — The bare if(user_f) requires T to be contextually convertible to bool, which fails to compile for lambda types or other callables without operator bool. Now conditional on std::is_constructible_v<bool, const T &>.

Performance

• Notify outside lock in submit_task — Eliminates hurry-up-and-wait where a woken worker immediately blocks on the still-held mutex.
• Remove lock re-acquisition for end_task_notifier in worker loop — cv::notify_one does not require the mutex.
• notify_one instead of notify_all for end_task_notifier — only the destructor waits on it.

Simplification

• Task extraction: replace temporary std::list + splice with std::move(queue.front()) + pop_front().
• Destructor: remove the while(!l_should_quit){ ... break; } pattern (single-iteration loop) in favor of a scoped lock block.
• Replace unused <algorithm> include with <type_traits>.
• Fix stale header comment (Thread_Pool.h → YgorThreadPool.h).

Tests

New tests2/YgorThreadPool.cc with subcases for: basic execution, single-worker FIFO ordering, std::exception handling, non-std exception handling, clear_tasks, 1000-task throughput, empty std::function, default/zero worker count, and recursive task submission from within tasks. All 6683 existing convex hull assertions (the primary work_queue consumer) continue to pass.

Original prompt

Ygor contains a simplistic work_queue thread pool class in src/Thread_Pool.h. This is a core building block class that many other methods and algorithms are based upon.

Evaluate the implementation for correctness and functionality. Focus on validity of the implementation. Also investigate performance, and suggest improvements to the implementation that can be guaranteed to be valid and bug-free.

Adhere to the local styles and conventions. All new C++ should strictly adhere to the C++17 standard.

---

💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.

[Copilot]

Pull request overview

This PR audits and updates the work_queue thread pool (src/YgorThreadPool.h) to improve correctness and performance, and adds a dedicated tests2 suite to validate behavior and regressions.

Changes:

• Update work_queue worker loop and shutdown logic (exception handling, task extraction, and condition-variable notification patterns).
• Clean up header includes/comment and add type_traits-based callable validity guarding.
• Add tests2/YgorThreadPool.cc and wire it into tests2/compile_and_run.sh.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/YgorThreadPool.h	Thread pool worker/destructor logic adjustments; exception handling changes; include changes.
tests2/YgorThreadPool.cc	New doctest suite covering core thread-pool behaviors (execution, ordering, exception swallowing, clear_tasks, etc.).
tests2/compile_and_run.sh	Adds the new thread-pool test translation unit to the explicit tests2 build list.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

YgorThreadPool.h uses std::chrono::{nanoseconds,seconds,milliseconds} but does not include <chrono>. The header should be self-contained; relying on indirect includes from <condition_variable> is non-portable. Add #include <chrono> here.

Suggested change

	#include <type_traits>
	#include <type_traits>
	#include <chrono>

[Copilot]

The clear_tasks subcase uses unbounded busy-wait loops (while(blocker.load()) ... and while(!started.load()) ...). If something regresses (e.g., worker never starts), the test can hang indefinitely and stall CI. Add a timeout/deadline (failing the test if exceeded) or use a condition_variable to wait for started/blocker transitions.

Suggested change

	while(blocker.load()) std::this_thread::sleep_for(std::chrono::milliseconds(10));
	});
	// Wait for the blocking task to start.
	while(!started.load()) std::this_thread::sleep_for(std::chrono::milliseconds(1));
	auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5);
	while (blocker.load() && std::chrono::steady_clock::now() < deadline) {
	std::this_thread::sleep_for(std::chrono::milliseconds(10));
	}
	});
	// Wait for the blocking task to start (with timeout to avoid hanging).
	auto start_deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5);
	while (!started.load() && std::chrono::steady_clock::now() < start_deadline) {
	std::this_thread::sleep_for(std::chrono::milliseconds(1));
	}
	REQUIRE(started.load());

[Copilot]

There is a stray null statement after the catch(...) block (catch(...){ };). It’s harmless but inconsistent with the preceding catch and makes the exception handling look accidental. Remove the extra ; (and consider formatting the two catch clauses consistently).

Suggested change

	}catch(const std::exception &){}
	catch(...){};
	} catch (const std::exception &) {
	} catch (...) {
	}