Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

The ultimate thread pool

423 views
Skip to first unread message

Bonita Montero

unread,
Jan 25, 2024, 12:26:05 PMJan 25
to

Once I've written a thread pool that has an upper limit of the number
threads and a timeout when idle threads end theirselfes. If you have
sth userpace CPU bound you'd specify the number of hardware-threads
as the upper limit, if you have much threads doing I/O you may go far
beyond since the hardware-threads aren't fully occupied anyway.
The problem with my initial thread pool class was that there may be
a large number of idle threads which could be used by other pools.
So I wrote a thread pool class where each pool has an upper limit of
the number of executing threads and there are no idle threads within
each pool. Instead the threads go idling in a global singleton pool
and attach to each pool which needs a new thread, thereby minimizing
the total number of threads.

This is the implementation

// header

#pragma once
#include <thread>
#include <mutex>
#include <condition_variable>
#include <deque>
#include <functional>
#include <chrono>

struct thread_pool
{
using void_fn = std::function<void ()>;
thread_pool( size_t maxThreads = 0 );
thread_pool( thread_pool const & ) = delete;
void operator =( thread_pool const & ) = delete;
~thread_pool();
uint64_t enqueue_task( void_fn &&task );
void_fn cancel( uint64_t queueId );
void wait_idle();
size_t max_threads();
size_t resize( size_t maxThreads );
bool clear_queue();
void_fn idle_callback( void_fn &&fn = {} );
std::pair<size_t, size_t> processing();
static typename std::chrono::milliseconds timeout(
std::chrono::milliseconds timeout );
private:
struct idle_node
{
idle_node *next;
bool notify;
};
using queue_item = std::pair<uint64_t, void_fn>;
using task_queue_t = std::deque<queue_item>;
bool m_quit;
size_t
m_maxThreads,
m_nThreadsExecuting;
uint64_t
m_lastIdleQueueId,
m_nextQueueId;
task_queue_t m_queue;
std::condition_variable m_idleCv;
std::shared_ptr<void_fn> m_idleCallback;
idle_node *m_idleList;
inline static struct global_t
{
std::mutex m_mtx;
std::chrono::milliseconds m_timeout = std::chrono::seconds( 1 );
std::condition_variable
m_cv,
m_quitCv;
bool m_quit;
size_t
m_nThreads,
m_nThreadsActive;
std::deque<thread_pool *> m_initiate;
void theThread();
global_t();
~global_t();
} global;
void processIdle( std::unique_lock<std::mutex> &lock );
std::unique_lock<std::mutex> waitIdle();
};

// translation unit

#include <cassert>
#include "thread_pool.h"
#include "invoke_on_destruct.h"

#if defined(_WIN32)
#pragma warning(disable: 26110) // Caller failing to hold lock
'lock' before calling function 'func'.
#pragma warning(disable: 26111) // Caller failing to release lock
'lock' before calling function 'func'.
#pragma warning(disable: 26115) // Failing to release lock 'lock'
in function 'func'.
#pragma warning(disable: 26117) // Releasing unheld lock 'lock' in
function 'func'.
#pragma warning(disable: 26800) // Use of a moved from object:
'object'.
#endif
#if defined(__llvm__)
#pragma clang diagnostic ignored "-Wparentheses"
#pragma clang diagnostic ignored "-Wunqualified-std-cast-call"
#endif

using namespace std;
using namespace chrono;

thread_pool::thread_pool( size_t maxThreads ) :
m_quit( false ),
m_maxThreads( [&]()
{
if( size_t hc; !maxThreads )
hc = jthread::hardware_concurrency(),
maxThreads = hc ? hc : 1;
return maxThreads;
}() ),
m_nThreadsExecuting( 0 ),
m_lastIdleQueueId( 0 ),
m_nextQueueId( 0 ),
m_idleList( nullptr )
{
}

// may throw system_error

thread_pool::~thread_pool()
{
// wait for idling and inherit the lock
unique_lock lock( waitIdle() );
// erase all outstanding spurious initiations to our pool;
// spurious because of stolen wakeups, non-cancelled initiations or
queue-clears
erase( global.m_initiate, this );
}

void thread_pool::global_t::theThread()
{
try
{
unique_lock lock( m_mtx );
// the first thread never waits with a timeout
bool reserve = m_nThreads == 1;
// reseved timeout which determines the beginning of a wait period
constexpr auto BEGIN_WAIT = time_point<steady_clock>(
steady_clock::duration( -1 ) );
auto start = BEGIN_WAIT;
for( ; ; )
{
while( !m_initiate.size() )
{
// quitting ?
if( m_quit )
{
// the initiation-queue is empty, we can quit;
// we're the last thread ?
if( !--m_nThreads )
// yes: notify the quitting thread's destructor
m_quitCv.notify_one();
return;
}
// it's our first round waiting, i.e. we're not coming
from a spurious initiation ?
if( start.time_since_epoch().count() < 0 )
// yes: set wating start time
start = steady_clock::now();
// we don't have any timeout or we're the reserve-thread ?
if( m_timeout.count() <= 0 || reserve )
// yes: wait for condition without timeout
m_cv.wait( lock );
else
{
// wait for condition with timeout;
// timeout and there are surplus threads and we're
not quitting ?
auto availAssert = []( size_t a ) {
assert((ptrdiff_t)a >= 0); return a; };
if( m_cv.wait_until( lock, start + m_timeout ) !=
cv_status::no_timeout
&& availAssert( m_nThreads - m_nThreadsActive )
> m_initiate.size() && !m_quit )
// yes: release thread
return (void)--m_nThreads;
}
}
// pop first initiate-entry
thread_pool *pool = m_initiate.front();
m_initiate.pop_front();
// pool is empty (spurious or stolen wakeup) or there are
enough running
// threads because the maximum number of threads has been
changed meanwhile ?
if( !pool->m_queue.size() || pool->m_nThreadsExecuting >=
pool->m_maxThreads )
// continue to wait at last start, keeping timeout due time
continue;
// our thread is marked running globally
++m_nThreadsActive;
// our thread is running inside a pool
++pool->m_nThreadsExecuting;
do
{
// pop worker-function
void_fn fn( move( pool->m_queue.front().second ) );
pool->m_queue.pop_front();
// call worker-item unlocked
lock.unlock();
fn();
lock.lock();
// repeat while pool still has items and the maximum
number of threads isn't exhausted;
// both pool-values could have changed while we were
unlocked
} while( pool->m_queue.size() && pool->m_nThreadsExecuting
<= pool->m_maxThreads );
// our thread is removed from the pool
--pool->m_nThreadsExecuting;
// our thread is available again
--m_nThreadsActive;
// ilde-notify
pool->processIdle( lock );
if( !lock )
lock.lock();
// continue with new timeout
start = BEGIN_WAIT;
}
}
catch( ... )
{
terminate();
}
}

void thread_pool::processIdle( std::unique_lock<mutex> &lock )
{
assert(lock);
// we're idling ?
if( m_queue.size() || m_nThreadsExecuting )
// no idle processing
return;
// update idle queue-id
m_lastIdleQueueId = m_nextQueueId;
// there are waiting synchronous idlers in the idle-list ?
if( idle_node *idler = m_idleList; idler )
{
// notify all synchronous idlers
do
idler->notify = true;
while( idler = idler->next );
// doesn't throw
m_idleCv.notify_all();
// idle-list is empty now
m_idleList = nullptr;
}
// there's no asynchronous idle-callback ?
if( !m_idleCallback )
return;
// lock current idle-pointer
shared_ptr<void_fn> idleCallback( m_idleCallback );
lock.unlock();
// call asynchronous idler
try
{
(*idleCallback)();
}
catch( ... )
{
terminate();
}
}

// may throw system_error

void thread_pool::wait_idle()
{
(void)waitIdle();
}

// may throw system_error

unique_lock<mutex> thread_pool::waitIdle()
{
unique_lock lock( global.m_mtx );
// there are no running threads in our pool and the queue is empty ?
if( !m_nThreadsExecuting && !m_queue.size() )
// we're allready idling
return lock;
// register idle-node
idle_node idleNode { m_idleList, false };
m_idleList = &idleNode;
// wait for idle-notification
do
// doesn't throw
m_idleCv.wait( lock );
while( !idleNode.notify );
return lock;
}


// may throw system_error
// may throw bad_alloc

uint64_t thread_pool::enqueue_task( void_fn &&task )
{
unique_lock lock( global.m_mtx );
assert(m_maxThreads);
// get next queue-id
uint64_t queueId = m_nextQueueId++;
// revert queue-id on exception
invoke_on_destruct revertQueueId( [&] { --m_nextQueueId; } );
// eneue ...
m_queue.emplace_back( queueId, move( task ) );
// enough threads running ?
if( m_nThreadsExecuting >= m_maxThreads )
// yes: no need to notify thread from global pool
return revertQueueId.disable(), queueId;
invoke_on_destruct moveBackTask( [&] { task = move(
m_queue.back().second ); m_queue.pop_back(); } );
// enqueue new initiation
global.m_initiate.emplace_back( this );
global.m_cv.notify_one();
// initiation succeeded, there'll be at least one thread
// which will process it, so disable reverting the initiation
moveBackTask.disable();
revertQueueId.disable();
assert(global.m_nThreads >= global.m_nThreadsActive);
// are there less free threads than initiations ?
if( global.m_nThreads - global.m_nThreadsActive >=
global.m_initiate.size() )
// no: no need to create addtional thread
return queueId;
// assure there's at least the reserve thread alive
assert(global.m_nThreads);
// yes: try to spawn new thread unlocked since this may take a
longer time and we won't lock the queues meanwhile;
// there is at least one thread, so the thead-counter can be
inacurrate since thread-creation may fail
++global.m_nThreads;
lock.unlock();
try
{
// create and detach pool-thread ?
thread( &global_t::theThread, &global ).detach();
}
catch( ... )
{
// failed: re-adjust threads counter;
// at least the reserve-thread is dedicated to us
lock.lock(),
--global.m_nThreads;
}
return queueId;
}

typename thread_pool::void_fn thread_pool::cancel( uint64_t queueId )
{
unique_lock<mutex> lock( global.m_mtx );
// reverse-find entry to cancel
auto cancelThis = find_if( m_queue.rbegin(), m_queue.rend(), [&](
pair<uint64_t, void_fn> &q ) { return q.first == queueId; } ).base();
// not found ?
if( cancelThis == m_queue.begin() )
// entry couldn't be cancelled
return void_fn();
// save function object for return-value
void_fn savedFn( std::move( (--cancelThis)->second ) );
// erase queue-item;
// leave initiation alive, the thread is already signalled anyway
and will remove the
// initiations in-order while we would remove them within the
initiation-queue (more expensive)
m_queue.erase( cancelThis );
// idle-notify
processIdle( lock );
return savedFn;
}

// may throw system_error

size_t thread_pool::max_threads()
{
lock_guard lock( global.m_mtx );
return m_maxThreads;
}

// may throw system_error

size_t thread_pool::resize( size_t maxThreads )
{
unique_lock lock( global.m_mtx );
// remember before number of maximum threads
size_t before = m_maxThreads;
// maximum number of threads determined by hardware concurrency ?
if( size_t hc; maxThreads )
// no: set max threads according to given value
m_maxThreads = maxThreads;
else
// yes: set max threads according to hardware concurrency
hc = jthread::hardware_concurrency(),
// if hardware concurrency is zero, use at least one thread
m_maxThreads = hc ? hc : 1;
// number of max threads has shrunken ?
if( maxThreads <= before )
// yes: no need to notify additional threads
return before;
// additional initiations over allowed initiations before;
// "before - m_nThreadsExecuting" may be negative because of recent
resizes
size_t addInitiate = m_queue.size() - (before - m_nThreadsExecuting);
// enough threads before ?
if( (ptrdiff_t)addInitiate <= 0 )
// yes: no need for additional initiations
return before;
// clip additional initiatons to maximum number of threads
size_t maxAddThreads = maxThreads - before;
addInitiate = addInitiate <= maxAddThreads ? addInitiate :
maxAddThreads;
// issue additiona initiations
auto &init = global.m_initiate;
for( size_t remainingInitiations = addInitiate;
remainingInitiations; --remainingInitiations )
{
try
{
// append initiation to the global initiation queue
init.emplace_back( this );
}
catch( ... )
{
// failed: we had at least one initiation or there was an
initiation to us before ?
if( remainingInitiations != addInitiate || find(
init.begin(), init.end(), this ) != init.end() )
{
// yes: that's sufficient, at least one thread is
dedicated to us;
// rebember number of successful innitiations
addInitiate -= remainingInitiations;
break;
}
else
// no: there's no initiation to us in the global queue;
// rethrow exception
throw;
}
global.m_cv.notify_one();
}
// additional thresds to create
assert(global.m_nThreads >= global.m_nThreadsActive);
size_t addCreate = addInitiate - (global.m_nThreads -
global.m_nThreadsActive);
if( (ptrdiff_t)addCreate <= 0 )
// no: let available threads do the work
return before;
// increase the number of threads to be created, creation may fail,
number may become inaccurate
global.m_nThreads += addCreate;
// create threads unlocked
lock.unlock();
for( ; addCreate; --addCreate )
try
{
// exception while creating thread ?
thread( &global_t::theThread, &global ).detach();
}
catch( ... )
{
// yes: there'll at least the reserve thread dedicated to us;
// adjust the threads counter, making it accurate again
lock.lock(),
global.m_nThreads -= addCreate;
break;
}
// threads counter is accurate now, return the before number of threads
return before;
}

// may throw system_error

bool thread_pool::clear_queue()
{
unique_lock lock( global.m_mtx );
// the queue is alreay empty ?
if( !m_queue.size() )
// yes: nothing to do
return false;
// clear the queue, leaving initiations alive since the thread is
awakened
// anyway and it's more efficient to remove the initiations from
the left
m_queue.clear();
// idle-notify
processIdle( lock );
return true;
}

// may throw system_error
// may throw bad_alloc

typename thread_pool::void_fn thread_pool::idle_callback( void_fn &&fn )
{
// create the new shared_ptr to the idle-callback unlocked
shared_ptr<void_fn> ptr;
if( fn )
ptr = make_shared<void_fn>( move( fn ) );
// apply the new idle-callback locked
lock_guard lock( global.m_mtx );
swap( ptr, m_idleCallback );
return move( *ptr );
}

// may throw system_error

pair<size_t, size_t> thread_pool::processing()
{
lock_guard lock( global.m_mtx );
return { m_queue.size(), m_nThreadsExecuting };
}

thread_pool::global_t::global_t() :
m_quit( false ),
m_nThreads( 1 ),
m_nThreadsActive( 0 )
{
thread( &global_t::theThread, this ).detach();
}

thread_pool::global_t::~global_t()
{
unique_lock lock( m_mtx );
// set quit flag for the threads waiting for initiations;
// remaining initiations are processed before quitting
m_quit = true;
// wait for all threads to quit
while( m_nThreads )
// notify_all() and wait() don't throw
m_cv.notify_all(),
m_quitCv.wait( lock );
}

typename std::chrono::milliseconds thread_pool::timeout(
std::chrono::milliseconds timeout )
{
lock_guard lock( global.m_mtx );
auto before = global.m_timeout;
global.m_timeout = timeout;
// awake all threads in global pool to adjust their due time
global.m_cv.notify_all();
return before;
}

Issuing a new thread is as easy as creating a normal C++11-thread:
pool.enqueue_task( functionObj [,... parameters] );
The pools maximum number of threads can be adjusted with resize().
Each task gets a 64 bit unique_id which is returned after enqueuing
a task. The task can be cancelled with thead_pool::cancel() if it
hasn't been fetched by a worker thread. The method wait_idle() waits
untill all tasks are executed. There's idle_callback() which is given
a function-object which is executed asynchronously in the last thread
before a pool is going to idle. The method processing() reports the
queue size and the number of currently executed tasks.

Chris M. Thomasson

unread,
Jan 25, 2024, 2:31:37 PMJan 25
to
On 1/25/2024 9:25 AM, Bonita Montero wrote:
>
> Once I've written a thread pool that has an upper limit of the number
> threads and a timeout when idle threads end theirselfes. If you have
> sth userpace CPU bound you'd specify the number of hardware-threads
> as the upper limit, if you have much threads doing I/O you may go far
> beyond since the hardware-threads aren't fully occupied anyway.
> The problem with my initial thread pool class was that there may be
> a large number of idle threads which could be used by other pools.
> So I wrote a thread pool class where each pool has an upper limit of
> the number of executing threads and there are no idle threads within
> each pool. Instead the threads go idling in a global singleton pool
> and attach to each pool which needs a new thread, thereby minimizing
> the total number of threads.
>
> This is the implementation
[...]

Just make sure to take the time to model it in a race detector.

Bonita Montero

unread,
Jan 25, 2024, 11:08:52 PMJan 25
to
Idiot ...


Chris M. Thomasson

unread,
Jan 26, 2024, 8:15:12 PMJan 26
to
Sigh. I don't have the time to look over your code and find any
potential issues right now. I will wait for one of your infamous
corrections instead. At least if you said here are some test units and
they pass, well, that would be a good sign, right? :^)

Chris M. Thomasson

unread,
Jan 26, 2024, 10:24:06 PMJan 26
to
On 1/25/2024 8:08 PM, Bonita Montero wrote:
Don't be ashamed of creating a test unit. If it find any errors, just
correct them, right? Notice how I formulated my xchg algortihm in a test
unit first!

https://groups.google.com/g/comp.lang.c++/c/Skv1PoQsUZo/m/bZoTXWDkAAAJ

No shame in that! Right? :^)

Chris M. Thomasson

unread,
Jan 26, 2024, 10:25:13 PMJan 26
to

Bonita Montero

unread,
Jan 27, 2024, 3:38:51 AMJan 27
to
Am 25.01.2024 um 20:31 schrieb Chris M. Thomasson:
The synchronization part is trivial.
It's the state the synchronization manages that is complex.

Chris M. Thomasson

unread,
Jan 27, 2024, 4:05:45 PMJan 27
to
On 1/25/2024 9:25 AM, Bonita Montero wrote:
>
> Once I've written a thread pool that has an upper limit of the number
> threads and a timeout when idle threads end theirselfes. If you have
> sth userpace CPU bound you'd specify the number of hardware-threads
> as the upper limit, if you have much threads doing I/O you may go far
> beyond since the hardware-threads aren't fully occupied anyway.
> The problem with my initial thread pool class was that there may be
> a large number of idle threads which could be used by other pools.
> So I wrote a thread pool class where each pool has an upper limit of
> the number of executing threads and there are no idle threads within
> each pool. Instead the threads go idling in a global singleton pool
> and attach to each pool which needs a new thread, thereby minimizing
> the total number of threads.
>
> This is the implementation
>
> // header
>
> #pragma once
> #include <thread>
> #include <mutex>
> #include <condition_variable>
> #include <deque>
> #include <functional>
> #include <chrono>
>
> struct thread_pool

[...]

>
> // translation unit
>
> #include <cassert>
> #include "thread_pool.h"
> #include "invoke_on_destruct.h"
>
> #if defined(_WIN32)
>     #pragma warning(disable: 26110) // Caller failing to hold lock
> 'lock' before calling function 'func'.
>     #pragma warning(disable: 26111) // Caller failing to release lock
> 'lock' before calling function 'func'.
>     #pragma warning(disable: 26115) // Failing to release lock 'lock'
> in function 'func'.
>     #pragma warning(disable: 26117) // Releasing unheld lock 'lock' in
> function 'func'.
>     #pragma warning(disable: 26800) // Use of a moved from object:
> 'object'.
> #endif
> #if defined(__llvm__)
>     #pragma clang diagnostic ignored "-Wparentheses"
>     #pragma clang diagnostic ignored "-Wunqualified-std-cast-call"
> #endif
[...]

Pardon my french, but what the heck is all of this for, exactly...

?

Chris M. Thomasson

unread,
Jan 28, 2024, 12:11:34 AMJan 28
to

Chris M. Thomasson

unread,
Jan 29, 2024, 5:45:17 PMJan 29
to
^^^^^^^^^^^^^

Why use _WIN32 here to disable all of those important warnings?

_MSC_VER instead?

Also, why disable all of those warnings in MSVC?

Bonita Montero

unread,
Jan 30, 2024, 5:45:13 AMJan 30
to
I could change that, but clang-cl is the only compiler that recoginizes
_MSC_VER and doesn't complain about the #pragmas.

Chris M. Thomasson

unread,
Jan 30, 2024, 6:19:19 PMJan 30
to
MSVC should define _MSC_VER, not exactly sure why clang-cl would be in
the mix. Probably due to:

https://clang.llvm.org/docs/MSVCCompatibility.html

Back in the day I used several compilers on Windows. Humm... Anyway,
what's up with all of those pragmas anyway? ;^)

Bonita Montero

unread,
Jan 31, 2024, 10:46:05 AMJan 31
to
Am 31.01.2024 um 00:19 schrieb Chris M. Thomasson:

> MSVC should define _MSC_VER, not exactly sure why clang-cl would be in
> the mix. Probably due to:

clang-cl does say its _MSC_VER, clang++ for Windows not.
clang-cl doesn't define __clang__, clang++ does.
But clang-cl does define __llvm__.

> https://clang.llvm.org/docs/MSVCCompatibility.html

Nothing of what I said.


Chris M. Thomasson

unread,
Jan 31, 2024, 3:32:14 PMJan 31
to
I think it is relevant. Also, what's up with all of those pragmas anyway?

Chris M. Thomasson

unread,
Jan 31, 2024, 9:35:35 PMJan 31
to

Chris M. Thomasson

unread,
Jan 31, 2024, 10:10:33 PMJan 31
to
Your pragmas also make me think of:

https://youtu.be/i_6zPIWQaUI

Bonita Montero

unread,
Feb 1, 2024, 5:18:52 AMFeb 1
to
Am 31.01.2024 um 21:32 schrieb Chris M. Thomasson:

> I think it is relevant. Also, what's up with all of those pragmas anyway?

They suppress warnings and the type of warning is in the commment.


Chris M. Thomasson

unread,
Feb 1, 2024, 3:05:47 PMFeb 1
to
Oh, I know all about that. But why in the world would anybody want to
suppress warnings that deal with:
_________________
#pragma warning(disable: 26110) // Caller failing to hold lock 'lock'
before calling function 'func'.

#pragma warning(disable: 26111) // Caller failing to release lock
'lock' before calling function 'func'.

#pragma warning(disable: 26115) // Failing to release lock 'lock'
in function 'func'.

#pragma warning(disable: 26117) // Releasing unheld lock 'lock' in
function 'func'.

#pragma warning(disable: 26800) // Use of a moved from object:
'object'.
_________________

Wow! ;^o

red floyd

unread,
Feb 1, 2024, 3:23:45 PMFeb 1
to
[SARCASM]
Because Bonita's code is perfect and you're a doofus, duh!
[/SARCASM]

Chris M. Thomasson

unread,
Feb 1, 2024, 3:31:02 PMFeb 1
to
YIKES! lol. Thanks for the laugh! :^D

These warnings are, ummm, well, _very_ important. I still don't know why
the Bonita suppresses them.


Bonita Montero

unread,
Feb 3, 2024, 2:04:46 PMFeb 3
to
Am 01.02.2024 um 21:05 schrieb Chris M. Thomasson:

> Oh, I know all about that. But why in the world would anybody want to
> suppress warnings that deal with:
> _________________
>  #pragma warning(disable: 26110) // Caller failing to hold lock 'lock'
> before calling function 'func'.
>
>      #pragma warning(disable: 26111) // Caller failing to release lock
> 'lock' before calling function 'func'.
>
>      #pragma warning(disable: 26115) // Failing to release lock 'lock'
> in function 'func'.
>
>      #pragma warning(disable: 26117) // Releasing unheld lock 'lock' in
> function 'func'.
>
>      #pragma warning(disable: 26800) // Use of a moved from object:
> 'object'.
> _________________
>
> Wow! ;^o

These warnings appear where they don't make sense, f.e. the compiler
doesn't see that I'm actually holding the lock.

Chris M. Thomasson

unread,
Feb 3, 2024, 3:05:41 PMFeb 3
to
Hummm... You threw me for a loop here... What exactly do you mean? Show
me a condensed example. Those warnings are there for a reason... You
really need to model it in a race detector. Relacy is a fun one.

Bonita Montero

unread,
Feb 3, 2024, 11:58:22 PMFeb 3
to
Am 03.02.2024 um 21:05 schrieb Chris M. Thomasson:

> Hummm... You threw me for a loop here... What exactly do you mean? Show
> me a condensed example. Those warnings are there for a reason... You
> really need to model it in a race detector. Relacy is a fun one.

The code doesn't need any race detector, the synchronization part is
trivial.

Chris M. Thomasson

unread,
Feb 4, 2024, 12:31:06 AMFeb 4
to
What happens if you turn off all of those very important warnings?

Bonita Montero

unread,
Feb 4, 2024, 1:42:03 AMFeb 4
to
What a question ...
I get incorrect warnings while compiling.

Chris M. Thomasson

unread,
Feb 4, 2024, 4:59:36 PMFeb 4
to
Why are they incorrect, according to you? Boil it down to a simple
example where these warnings are incorrect. That way I can reproduce it
on my side.

Chris M. Thomasson

unread,
Feb 4, 2024, 11:20:21 PMFeb 4
to

Bonita Montero

unread,
Feb 5, 2024, 3:02:07 AMFeb 5
to
Am 04.02.2024 um 22:59 schrieb Chris M. Thomasson:

> Why are they incorrect, according to you? ...

No, according to what they should warn for.

Chris M. Thomasson

unread,
Feb 5, 2024, 2:09:32 PMFeb 5
to
Can you create a little self-contained example that shows these warnings?

Bonita Montero

unread,
Feb 6, 2024, 1:12:26 AMFeb 6
to
Am 05.02.2024 um 20:09 schrieb Chris M. Thomasson:

> Can you create a little self-contained example that shows these warnings?

Make the pragmas commented and look what the IDE shows as warnings.

Chris M. Thomasson

unread,
Feb 6, 2024, 1:35:02 AMFeb 6
to
What is in:
______________________
#include <cassert>
#include "thread_pool.h"
#include "invoke_on_destruct.h"
______________________

?

Chris M. Thomasson

unread,
Feb 6, 2024, 1:36:57 AMFeb 6
to
You have some issues here. These warnings are not for kicks and giggles,
so to speak. If these warnings are wrong, you need to file a bug report
to the MSVC team.

Chris M. Thomasson

unread,
Feb 6, 2024, 1:38:25 AMFeb 6
to
On 2/5/2024 10:12 PM, Bonita Montero wrote:
Think about getting a warning free compile. Think about what these
warnings actually mean. If you find a bug, report it to the MSVC team.

Chris M. Thomasson

unread,
Feb 6, 2024, 1:46:20 AMFeb 6
to
^^^^^^^^^^^^^^^^^^^^^^

What is that for?

> ______________________
>
> ?

Bonita Montero

unread,
Feb 6, 2024, 5:16:03 AMFeb 6
to
Am 06.02.2024 um 07:34 schrieb Chris M. Thomasson:
> On 2/5/2024 10:12 PM, Bonita Montero wrote:
>> Am 05.02.2024 um 20:09 schrieb Chris M. Thomasson:
>>
>>> Can you create a little self-contained example that shows these
>>> warnings?
>>
>> Make the pragmas commented and look what the IDE shows as warnings.
>>
>
> What is in:
> ______________________
> #include <cassert>
> #include "thread_pool.h"

The header I posted.

> #include "invoke_on_destruct.h"

Sth. like experimental::scope_exit, this is my code:

#pragma once
#include <utility>

template<typename Fn>
requires requires( Fn fn ) { { fn() }; }
struct invoke_on_destruct
{
private:
Fn m_fn;
bool m_enabled;
public:
invoke_on_destruct( Fn &&fn ) :
m_fn( std::forward<Fn>( fn ) ),
m_enabled( true )
{
}
~invoke_on_destruct()
{
if( m_enabled )
m_fn();
}
void operator ()()
{
if( m_enabled )
m_enabled = false,
m_fn();
}
void disable()
{
m_enabled = false;
}
invoke_on_destruct &enable()
{
m_enabled = true;
return *this;
}
};

Bonita Montero

unread,
Feb 6, 2024, 5:17:16 AMFeb 6
to
Am 06.02.2024 um 07:38 schrieb Chris M. Thomasson:

> Think about getting a warning free compile. Think about what these
> warnings actually mean. If you find a bug, report it to the MSVC team.

Where the compiler warns me there are no errors,
these are false warnings.

Bonita Montero

unread,
Feb 6, 2024, 8:01:49 AMFeb 6
to
Am 06.02.2024 um 07:38 schrieb Chris M. Thomasson:
Try this ...

void fn( unique_lock<mutex> &lock )
{
assert(lock);
lock.unlock();
}

... and you'll get a warning.

Chris M. Thomasson

unread,
Feb 6, 2024, 2:56:05 PMFeb 6
to
Well, that deserves a bug report?

Chris M. Thomasson

unread,
Feb 6, 2024, 3:07:29 PMFeb 6
to
Fwiw, I get no warning with:

{
std::unique_lock<std::mutex> lock(m_mutex);
assert(lock);
lock.unlock();
//lock.unlock(); // this throws an exception.
}

The interesting part is the code for std::unique_lock:

~unique_lock() noexcept {
if (_Owns) {
_Pmtx->unlock();
}
}


I was wondering why std::unique_lock did not try to unlock it twice in
the dtor. Well, the code for std::~unique_lock that MSVC is using
answers my question. Its that _Owns variable. Humm...

Bonita Montero

unread,
Feb 7, 2024, 11:13:50 AMFeb 7
to
I won't file it.

Bonita Montero

unread,
Feb 7, 2024, 11:15:11 AMFeb 7
to
Am 06.02.2024 um 21:07 schrieb Chris M. Thomasson:
> On 2/6/2024 5:01 AM, Bonita Montero wrote:
>> Am 06.02.2024 um 07:38 schrieb Chris M. Thomasson:
>>> On 2/5/2024 10:12 PM, Bonita Montero wrote:
>>>> Am 05.02.2024 um 20:09 schrieb Chris M. Thomasson:
>>>>
>>>>> Can you create a little self-contained example that shows these
>>>>> warnings?
>>>>
>>>> Make the pragmas commented and look what the IDE shows as warnings.
>>>>
>>>
>>> Think about getting a warning free compile. Think about what these
>>> warnings actually mean. If you find a bug, report it to the MSVC team.
>>
>> Try this ...
>>
>> void fn( unique_lock<mutex> &lock )
>> {
>>      assert(lock);
>>      lock.unlock();
>> }
>>
>> ... and you'll get a warning.
>>
>
> Fwiw, I get no warning with:
>
> {
>     std::unique_lock<std::mutex> lock(m_mutex);
>     assert(lock);
>     lock.unlock();
>     //lock.unlock(); // this throws an exception.
> }

That's sth. totally different. My function has the pre-condition
that it inherits the lock of the calling function and sometimes
it unlocks it.

Chris M. Thomasson

unread,
Feb 7, 2024, 3:38:24 PMFeb 7
to
On 2/7/2024 8:15 AM, Bonita Montero wrote:
> Am 06.02.2024 um 21:07 schrieb Chris M. Thomasson:
>> On 2/6/2024 5:01 AM, Bonita Montero wrote:
>>> Am 06.02.2024 um 07:38 schrieb Chris M. Thomasson:
>>>> On 2/5/2024 10:12 PM, Bonita Montero wrote:
>>>>> Am 05.02.2024 um 20:09 schrieb Chris M. Thomasson:
>>>>>
>>>>>> Can you create a little self-contained example that shows these
>>>>>> warnings?
>>>>>
>>>>> Make the pragmas commented and look what the IDE shows as warnings.
>>>>>
>>>>
>>>> Think about getting a warning free compile. Think about what these
>>>> warnings actually mean. If you find a bug, report it to the MSVC team.
>>>
>>> Try this ...
>>>
>>> void fn( unique_lock<mutex> &lock )
>>> {
>>>      assert(lock);
>>>      lock.unlock();
>>> }
>>>
>>> ... and you'll get a warning.
>>>
>>
>> Fwiw, I get no warning with:
>>
>> {
>>      std::unique_lock<std::mutex> lock(m_mutex);
>>      assert(lock);
>>      lock.unlock();
>>      //lock.unlock(); // this throws an exception.
>> }
>
> That's sth. totally different. My function has the pre-condition
> that it inherits the lock of the calling function and sometimes
> it unlocks it.

Ahhh. I got them with:
__________________________________
namespace ct
{
struct mutex_test
{
std::mutex m_mutex;

void
bar(std::unique_lock<std::mutex>& lock)
{
assert(lock); // better be locked!
lock.unlock();
}

void
foo()
{
std::unique_lock<std::mutex> lock(m_mutex);
bar(lock); // beware. unlocks the damn thing!
lock.lock(); // okay...
}
};
}
__________________________________

Give the warnings:
__________________________________
Severity Code Description Project File Line Suppression State Details
Warning C26115 Failing to release lock 'this->m_mutex' in function
'ct::mutex_test::foo'. ct_threads
D:\ct_dev\projects\ct_threads\ct_threads\ct_main.cpp 26

Warning C26111 Caller failing to release lock 'this->m_mutex' before
calling function 'std::unique_lock<std::mutex>::lock'. ct_threads
D:\ct_dev\projects\ct_threads\ct_threads\ct_main.cpp 26

Warning C26110 Caller failing to hold lock 'lock' before calling
function 'std::unique_lock<std::mutex>::unlock'. ct_threads
D:\ct_dev\projects\ct_threads\ct_threads\ct_main.cpp 18
__________________________________


You just have to be very careful with this type of logic. These warnings
elude to that.



[...]

Chris M. Thomasson

unread,
Feb 7, 2024, 3:41:25 PMFeb 7
to
Maybe you should. However, the warnings do flag some "sketchy" things
are going on.

Bonita Montero

unread,
Feb 8, 2024, 7:07:13 AMFeb 8
to
Can't you read my code ?
That's still sth. completely different than what I did.

Chris M. Thomasson

unread,
Feb 8, 2024, 3:03:04 PMFeb 8
to
Yes.


> That's still sth. completely different than what I did.
>

I just wanted to get MSVC to pop out those "false" warnings. Mission
accomplished.

Chris M. Thomasson

unread,
Feb 8, 2024, 4:28:55 PMFeb 8
to
Keep in mind that your code disables those warnings. So, be sure to turn
them back on for any and all user code!

Bonita Montero

unread,
Feb 9, 2024, 7:17:08 AMFeb 9
to
Am 08.02.2024 um 22:28 schrieb Chris M. Thomasson:

> Keep in mind that your code disables those warnings. So, be sure to turn
> them back on for any and all user code!

Locking and unlocking mutexes is trivial and I don't need any addtional
aid for that. And MSVC runtime gives me a debug message if I try to
unlock a non-locked mutex; that's sufficient for me. And the shown
code is never trapped in that way.

Chris M. Thomasson

unread,
Feb 9, 2024, 2:43:39 PMFeb 9
to
I am saying that a user of your code might not want those warnings
disabled at all. So, you should turn them back on.

Bonita Montero

unread,
Feb 10, 2024, 12:11:34 AMFeb 10
to
Am 09.02.2024 um 20:43 schrieb Chris M. Thomasson:

> I am saying that a user of your code might not want those
> warnings disabled at all. So, you should turn them back on.

In my own translation units I don't re-enable those warnings.
In headers I disable certain warnings at the beginning of the
header and re-enable them at the end, so that these headers
won't disable those warnings for translation u