I reply in series below to Jason and Federico.


On Thu, Jun 29, 2023 at 12:56 AM Jason McKesson wrote:
>
> The first principle of good thread-safe code is to not share stuff
> across threads. If something must be shared across threads, it should
> be designed to be shared for that particular purpose. If your use of
> shared objects is so unstructured that you can't even stop *your own
> thread* from double-locking such an object, then it seems pretty
> unlikely you're writing good code.


What about encapsulation?

Let's say I have a class that manages the network card, sending &
receiving packets. Then I have another class managing the RS232
connection.

One of my worker threads reads from the network card, processes the
data and forwards it on to the RS232 port. Having forwarded the
packet, the worker thread adds the hashsum of the packet to a global
container (along with a boolean to say whether or not it's been
forwarded yet). The container is defined as:

    std::map< __uint128_t, bool > g_hashes;

The main GUI thread has a timer that fires every 200 milliseconds,
which prints the size of 'g_hashes' to the screen. So I need
synchronisation here. So I change the definition of that global
container:

    Reservable< std::map< __uint128_t, bool > > g_hashes;

Then in my network card class, I have something like:

    bool NetworkCard::GetPacket(void)
    {
        ssize_t const count = ::recv( this->fd, this->buf,
sizeof(this->buf), 0 );

        if ( 0 == count ) return false;

        auto mylock = g_hashes.Reserve();
        auto &myhashes = *mylock;
        myhashes[ CalculateHashsum(this->buf, count) ] = false; //
'false' means hasn't been forwarded yet
        gRS232->ForwardPacket(this->buf, count);
        // Do some other stuff here before releasing the lock on g_hashes
        return true;
    }

And then in my RS232 class, I have:

    bool RS232::ForwardPacket(char const *const p, std::size_t const len)
    {
        __uint128_t const hashsum = CalculateHashsum(p, len);

        auto mylock = g_hashes.Reserve();
        auto &myhashes = *mylock;

        if ( myhashes.end() == myhashes.find(hashsum) ) return false;

        assert( false == myhashes[hashsum] );

        if ( len == ::send(this->fd, p, len,0) )
        {
            myhashes[hashsum] = true;
        }
    }

The RS232 class doesn't just forward packets from the network card; it
also forwards packets from the USB port and also from other RS232
ports. So the RS232 class doesn't know what those other classes are
doing nor what they're accessing or locking. The above code would
thread-lock if it weren't for the recursive_mutex used by Reservable.
I know you can argue that NetworkCard::GetPacket should release the
lock before calling RS232::ForwardPacket, and so maybe I didn't write
out a great example in the 10 minutes I took to write this, but I
think I did a good job of at least showing the principle that Class A
might hold onto the lock while it tells Class B to do something. Some
people think that this isn't good coding practise and that the
programmer should be more strict. Personally though I like the
flexibility of it all.

Moving on to Federico:
> In
>
> ----
> push_back(44);
> push_back(47);
> ----
>
> you are loking at every push_back, not ideal, and while looking at the
> code it is not obvious.
> Also you might have wanted 47 appearing in your vector after 44, since
> vector is "thread-safe", it is not an unfair assumption, but it is not
> necessarily true, as someone else might be pushing something in the
> meantime, and thus introduce a value between 44 and 47.

My code snippet doesn't lock at every 'push_back'. Take a second look
at it. It acquires the lock, pushes twice, then releases the lock. 47
will always go in right after 44 because of the lock.