the mining threads leak a mutex and there is unreasonable string::empty() usage

[cppdev-123]

in the work main function there is a mutex leak

in this code :

std::unique_lock<std::mutex> lck(thd_aff_set);
lck.release();  // -> mutex leak , this line should be removed so the mutex is unlocked on exit

if the thread exits after releasing the mutex and the class is destructed (deleted) in debug builds this will hit a break point saying : mutex destroyed while busy

also in the executor.cpp there are two uses for the method empty of the string class like this :

motd.empty();

note that this method is marked with nodiscard in c++17 to issue a compiler warning

also in the telemetry class the telem member of executor class is accessed by the ex_main thread only so why using an array for mutex (one per running backend) ?
the miner works well when these mutexes are removed from the code

[fireice-uk]

You refer to this by "mutex leak" ?

https://wiki.sei.cmu.edu/confluence/display/cplusplus/CON50-CPP.+Do+not+destroy+a+mutex+while+it+is+locked

Can you point to the line where you think the mutex is destroyed early?

note that this method is marked with nodiscard in c++17 to issue a compiler warning

Ah. That will be useful. Not the first time I wrote empty when I meant clear.

also in the telemetry class the telem member of executor class is accessed by the ex_main thread only so why using an array for mutex (one per running backend) ?

It is more useful if you link to exact places in the code when you discuss them.

[cppdev-123]

By mutex leak I mean a thread is terminated (joined) without unlocking a mutex it owns its lock

you prevent the thread from start mining before affinity setting by using a mutex
the minethd constructor locks the mutex with unique_lock and it is get unlocked automatically when the function returns

Now the work_main method owns the lock and starts to work :

std::unique_lock<std::mutex> lck(thd_aff_set);

but after this in the next line the mutex is released so the unique_lock no longer owns it and it has to be unlocked manually

lck.release();

according to this https://en.cppreference.com/w/cpp/thread/unique_lock/release
the mutex is not unlocked by using release and you now have the responsibility to unlock it

and from here https://en.cppreference.com/w/cpp/thread/mutex/~mutex
the behaviour is undefined if the thread terminates without unlocking the mutex it owns

so this line should be removed so unique_lock unlocks the mutex on exit or replaced by this :

lck.unlock();

For the telemetry it is accessed only by one thread which is ex_main
To access it outside this thread you use a queue of events which at the end is catched by ex_main thread and processed

So there is no need to use any mutex, and those are the mutexes I mean :
0840778

again no need for exclusive lock or shared lock
And if you need shared lock you had better use the one provided by the os
Windows has srw locks and pthread has rwlock and c++17 introduced shared_mutex which is based upon them

[fireice-uk]

but after this in the next line the mutex is released so the unique_lock no longer owns it and it has to be unlocked manually

Ah. Got it. Another empty vs clear :>

For the telemetry it is accessed only by one thread which is ex_main

@psychocrypt Did that lock fall out of favour, or is this something you never got round to?

And if you need shared lock you had better use the one provided by the os

C++14 feature I'm afraid. Back when I wrote this, most distros struggled with having compliant C++11 gcc version.

[psychocrypt]

thx for the bug report: first bug empty usage instead of clear() is fixed i n #2521

[psychocrypt]

@cppdev-123 you are right the mutex in telemetry is absolute bullshit and useless.
fixed in #2522

[psychocrypt]

fix the release issue #2530

[psychocrypt]

I close this issue because all reported problems should be fixed when #2530 is merged. Thanks again for reporting!