Re: [boost] [thread] Use of synchronized_value<> member in a constructor

Subject: Re: [boost] [thread] Use of synchronized_value<> member in a constructor
From: Klaim - Joël Lamotte (mjklaim_at_[hidden])
Date: 2013-06-13 06:43:17

On Thu, Jun 13, 2013 at 8:13 AM, Vicente J. Botet Escriba <
vicente.botet_at_[hidden]> wrote:
>
> Just to clarify, and make sure I understand 100% correctly,
>> are you talking about the member's construction (m_info) or the owning
>> object construction (Sequence)?
>>
> I meant the member.
>
>  I assume the later, so the member don't need locking until the end of the
>> constructor of the owning object.
>>
> No. Any access to the member would imply a lock after construction.
>
> m_info-> returns a lock.
>
>
Ok then.

>    I suspect that there might be other cases like this one where
>>>
>>>> the object responsable for the synchronized object should be able,
>>>> rarely,
>>>> to access the object unsafely.
>>>>
>>> You are supposing that there is an object responsible for synchronized
> object, but I don't know what this means and how to ensure it.
> If you want this pattern , you maybe need external locking. See the class
> externally_locked.
>
>
Ok I'll take a look.

>
>>>>  For example?
>>>
>>>
>>>  This is just speculation. Maybe I'm just pessimistic.
>> Currently I only have the case of the constructor and I don't think I will
>> have others.
>> I have another bigger code base which have more complex concurrent cases
>> where
>> I might find similar usage (access to a usually locked object in a case
>> where I am 100% sure there can't be concurrent accesses).
>>
> Could you develop the case and explain how are you 100% sure that there is
> no concurrent access?
>
>
I'll get back to this after looking at the codebase I was talking about.

>  I will search and report if I find something, I'm not sure I will.
>>
>>
>>  Suggestion: add an unsafe access to the object, in a very visible way.
>>>>
>>>>       m_info.unsafe_access().id( new_id );
>>>>
>>>> OR
>>>>
>>>>       m_info.unsafe_access()->id( new_id ); // using a smart pointer
>>>> which
>>>> checks if safe accesses have been done inbetween and throw if it is the
>>>> case? or any other checks detecting potential error
>>>>
>>> BTW, I don't know what "checks if safe accesses have been done
> inbetween" could mean? Could you clarify?
>
>
I am not sure really, I don't think I have enough experience in the domain.
I was thinking maybe there are ways to note if at least one lock have been
taken while
while accessing the object unsafely. Maybe just throw an exception on
locking if an unsafe access is going on?

>
>> However it is still not a perfect solution.
>> In the real code I have some logic I need to do before manipulating the
>> synchronized member.
>> It should be done in this order, first some logic, then use thesynched
>> object.
>> But even if I use a function to create the member but still have to access
>> it
>> later because of that logic, there is no point in doing that.
>>
> You can create a unsynchronized SequenceInfo object, do whatever you want
> with and last create the synchronized SequenceInfo just by copying.
>
>
Yes, when the object is copyable is less of a problem.

>  To clarify what I'm trying to explain (this is fictive code close to mine,
>> which is more complex):
>>
>> explicit Sequence( SequenceId new_id, SequenceInfo info )
>>            : m_info(  merge_info( new_id, info ) )      // assuming I
>> follow
>> your suggestion
>>      {
>>            if( first_thing_to_do() )
>>            {
>>               second_thing_to_do( m_info->name ); // still need to lock
>>            }
>>      }
>>
> I recognize that the factory function could be less natural, but it works
>
>
For this case I agree.

>
>
>  I'm using function name instead of real code just so you get the idea,
>> sorry if it's not real code.
>> I try to do minimum work in the constructor but I noticed the locking
>> access when I couldn't avoid it.
>>
>> I will try to be clearer with some code, first this:
>>
>> class InfoProcessor;    // generate analytic data from info -
>> non-copyable!
>>
> Is it movable?

In my case no. If it was it would be ok.

>
>> class Sequence
>> {
>>      boost::synchronized_value<**InfoProcessor> m_infoproc;
>>
>> public:
>>      explicit Sequence( SequenceInfo info )
>>            : m_infoproc(  info )      // process info into the
>> constructor,
>> ready to be used
>>
> Isn't the parameter  InfoProcessor info?

No, it's SequenceInfo. m_infoproc "unflat" data from the basic info
provided.

>       {
>>            m_infoproc->do_something(); // can't avoid a lock?
>>      }
>>
>>      Sequence( const Sequence& ) = delete;
>>      Sequence& operator=( const Sequence& ) = delete;
>>
>>
>> Assuming I can't modify InfoProcessor, the only way I see how to do the
>> call in the constructor
>> without locking would be to encapsulate the member into a wrapper:
>>
>> class Sequence
>> {
>>      struct InfoProcWrapper {
>>           InfoProcWrapper( SequenceInfo info )
>>               : infoproc( info )
>>           { infoproc.do_something(); }   // no locking
>>
>>           InfoProcessor infoproc;
>>      };
>>      boost::synchronized_value<**InfoProcWrapper > m_infoproc;
>>
>> public:
>>      explicit Sequence( SequenceInfo info )
>>            : m_infoproc(  info )      // now any access will need locking
>>      {
>>            // OK: m_infoproc->infoproc.do_**something() was already
>> called
>> without locking.
>>      }
>>
>> Which would be fine if it wasn't so noisy and that's only for 1
>> synchronized member...
>>
> I agree, creating these wrappers is too noisy but an alternative. I think
> the factory is a better choice. Note that you have lambdas in C++11.
>
>
Yes I thought about lambdas afterward, but assuming InforProc is not
movable nor copyable, the factory you are suggesting
would have to create it dynamically to be able to pass it around. It's
again a solution, not perfect for all cases but I guess that's ok in most
cases.

>
>
>
>> class Sequence
>> {
>>      struct InfoProcWrapper {
>>           InfoProcWrapper( SequenceInfo info )
>>               : infoproc( info )
>>           { infoproc.do_something(); }   // no locking
>>
>>           InfoProcessor infoproc;
>>      };
>>      struct DataCruncherWrapper {
>>           DataCruncherWrapper( SequenceInfo info )
>>               : cruncher( info )
>>           { cruncher.prepare( 42 ); }   // no locking
>>
>>           DataCruncher cruncher;
>>      };
>>      boost::synchronized_value<**InfoProcWrapper> m_infoproc;
>>      boost::synchronized_value<**DataCruncher> m_cruncher; // should NOT
>> be
>> synched with the same mutex
>>
>> public:
>>      explicit Sequence( SequenceInfo info )
>>            : m_infoproc(  info )      // OK: setup with no locking
>>            : m_cruncher(  info )      // OK: setup with no locking
>>      {
>>             m_cruncher->cruncher.**configure( m_infoproc->infoproc ); //
>> how
>> to do that without locking?
>>      }
>>
> Delaying the construction. This is similar to the problems raised for
> writing constexpr fuctions.
>
> class Sequence
> {
>   struct X {
>     InfoProcWrapper m_infoproc;
>     DataCruncher m_cruncher;
>     X(SequenceInfo info)
>           : sx_(  info )
>           : m_cruncher(  info )
>
>     {
>            m_cruncher->cruncher.**configure( m_infoproc->infoproc );
>     }
>  };
>
>   struct SX {
>
>     boost::synchronized_value<**InfoProcWrapper> m_infoproc;
>     boost::synchronized_value<**DataCruncher> m_cruncher; // should NOT be
> synched with the same mutex
>     SX(X) {...};
>   };
>     SX sx_;
>
>  public:
>     explicit Sequence( SequenceInfo info )
>           : sx_(  X(info) )
>
>     {
>     }
>
>
Ok but that works only if InforProc and DataCruncher are at least movable.
So, unperfect but works in a lot of cases.

>> Anyway, I might be wrong but it seems to me that any attempt to combine
>> use
>> of several synched members in the constructor
>> will imply locking OR adding several layers of abstractions, which if the
>> members' types have value-semantic, can be easy,
>> but if it's not the case, it becomes quickly very noisy.
>>
> I agree. We surely need something that make this easier and less noisy. I
> would prefer if we continue to see other alternatives to go directly to
> providing a bypass function.
>
>
Yes. I am thinking maybe part of the solution would be to have a second
optional argument in the synchronized_value<> constructor which would be
lambda
which would be called in the constructor body of the synchronized_value<>:

explicit Sequence( SequenceInfo info )
           : m_infoproc(  info , []( InfoProcessor& infoproc ){
infoproc.do_something(); } )    // no locking - lamda called in
synchronized_value<>
constructor

It don't solve the case where you want to use several members in the
Sequence construuctor though.
Is there any way in C++ to detect if we are in an object's construction? I
think not.
It would the opposite of the proposal for detecting when an exception have
been thrown and stack-unwinding is occuring.

I can't think of a better solution right now.

>> Note that hopefully the cost of the lock isn't important so far in my
>> specific use case of today,
>> I was just pointing the constructor cost because in the other code bases I
>> have
>> I might have more performance-related concerns and I might not be the only
>> one.
>>
>>
>>  I would help you if I can.
>
>
Thanks!

Joel Lamotte