On Thu, Jun 29, 2023 at 4:44 AM Frederick Virchanza Gotham via
Std-Proposals <std-proposals_at_[hidden]> wrote:
>
> 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 irony of starting a response with "What about encapsulation?" and
then having that example use a *global variable* is obviously lost on
you.

In any case, if your intent is to share this set of hashes with other
code (and it is a set, not a map. A port is either forwarded or not in
the list), then you need to sit down and think about how you want that
other code to use it. For example:

> 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.

The intent is for other code to be able to read it, not modify it. So
you should have a mechanism that supports this use case and *does not*
allow the object to be modified by just anybody.

That would be good encapsulation and good thread safety. Simply
shoving a container into a global variable paired with a mutex
(directly or indirectly as your proposal would allow) is neither of
those things.

> 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.

"Flexibility" is how you get spurious deadlocks that are insanely
difficult to debug. If your code is not *proveably* thread-safe, then
it isn't thread-safe at all.