You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Andrew Stitcher <as...@redhat.com> on 2008/10/06 17:38:45 UTC
Re: [c++] Experience with Alexandrescu's use of "volatile" to
enforce locking.
On Fri, 2008-10-03 at 17:31 -0400, Alan Conway wrote:
> ...
> (1) my usual style being:
> - all public member functions are thread safe.
> - use ScopedLock to acquire/release locks.
> - private, unlocked functions that are only intended to be called with
> the lock already held take a dummy extra paramater of type ScopedLock&
>
> The dummy ScopedLock& parameter marks functions that are not locked and
> also makes it hard to accidentally call them in an unlocked context.
This would explain why I occasionally see odd functions with an unused
ScopedLock& parameter, and try and figure out why the person who wrote
it did that. Then sigh, and go and remove the useless parameter before
checking in my changes.
Did I miss a wiki note/or an email? Oh, such is life.
Andrew
Re: [c++] Experience with Alexandrescu's use of "volatile" to enforce
locking.
Posted by Ted Ross <tr...@redhat.com>.
Andrew Stitcher wrote:
> On Mon, 2008-10-06 at 13:29 -0400, Alan Conway wrote:
>
>> On Mon, 2008-10-06 at 11:38 -0400, Andrew Stitcher wrote:
>>
>>> On Fri, 2008-10-03 at 17:31 -0400, Alan Conway wrote:
>>>
>>>> ...
>>>> (1) my usual style being:
>>>> - all public member functions are thread safe.
>>>> - use ScopedLock to acquire/release locks.
>>>> - private, unlocked functions that are only intended to be called with
>>>> the lock already held take a dummy extra paramater of type ScopedLock&
>>>>
>>>> The dummy ScopedLock& parameter marks functions that are not locked and
>>>> also makes it hard to accidentally call them in an unlocked context.
>>>>
>>> This would explain why I occasionally see odd functions with an unused
>>> ScopedLock& parameter, and try and figure out why the person who wrote
>>> it did that. Then sigh, and go and remove the useless parameter before
>>> checking in my changes.
>>>
>>> Did I miss a wiki note/or an email? Oh, such is life.
>>>
>> I generally stick a comment in the .h file but very possible I've
>> forgotten on occasion. I sent round an email when I first started doing
>> this, but that's easy to miss. I'll be more careful to include comments
>> in future.
>>
>> Any other useful conventions out there for marking thread safe/unsafe
>> functions? I think the public == thread safe is generally a good rule
>> but I've yet to find a satisfactory way of marking private thread-unsafe
>> functions as such.
>>
>
> Generally I just put a comment (in header file or both) saying that a
> prerequisite for this function is to be holding a particular lock.
>
> I think "the correct" way to do this is to use something like
> "assert(<lock held>);". I think this is the correct way as I think you
> should document (as many as possible) preconditions with asserts.
> However we don't have a way of expressing <lock held> efficiently as far
> as I know.
>
>
>
>
> Andrew
>
>
I've been using an "LH" suffix on the function name (i.e.
handleMethodRequestLH) to denote lock-held. This at least makes it easy
to visually inspect for lock problems.
-Ted
Re: [c++] Experience with Alexandrescu's use of "volatile" to
enforce locking.
Posted by Andrew Stitcher <as...@redhat.com>.
On Mon, 2008-10-06 at 16:36 -0400, Alan Conway wrote:
> On Mon, 2008-10-06 at 16:07 -0400, Andrew Stitcher wrote:
> > ...
> > - I'm willing to go along with it, I just happen to think that
> > using assert() is better in this case - especially as we have no low
> > cost way to do the assertion in any case.
>
> Confused me with that last sentence - the first part says we should use
> assert(), but the second part says we can't?
The point is essentially moot as we don't have a low cost way to test
whether a lock is held or not (trylock() as the only way we can
currently do it, and it can be expensive ISTR).
Being moot apparently didn't stop me sounding off about it anyway :)
A
Re: [c++] Experience with Alexandrescu's use of "volatile" to
enforce locking.
Posted by Alan Conway <ac...@redhat.com>.
On Mon, 2008-10-06 at 16:07 -0400, Andrew Stitcher wrote:
> On Mon, 2008-10-06 at 14:23 -0400, Alan Conway wrote:
> > On Mon, 2008-10-06 at 13:39 -0400, Andrew Stitcher wrote:
> > > On Mon, 2008-10-06 at 13:29 -0400, Alan Conway wrote:
> > > > On Mon, 2008-10-06 at 11:38 -0400, Andrew Stitcher wrote:
> > > > > On Fri, 2008-10-03 at 17:31 -0400, Alan Conway wrote:
> > > > > > ...
> > > > > > (1) my usual style being:
> > > > > > - all public member functions are thread safe.
> > > > > > - use ScopedLock to acquire/release locks.
> > > > > > - private, unlocked functions that are only intended to be called with
> > > > > > the lock already held take a dummy extra paramater of type ScopedLock&
> > > > > >
> > > > > > The dummy ScopedLock& parameter marks functions that are not locked and
> > > > > > also makes it hard to accidentally call them in an unlocked context.
> > > > >
> > > > > This would explain why I occasionally see odd functions with an unused
> > > > > ScopedLock& parameter, and try and figure out why the person who wrote
> > > > > it did that. Then sigh, and go and remove the useless parameter before
> > > > > checking in my changes.
> > > > >
> > > > > Did I miss a wiki note/or an email? Oh, such is life.
> > > >
> > > > I generally stick a comment in the .h file but very possible I've
> > > > forgotten on occasion. I sent round an email when I first started doing
> > > > this, but that's easy to miss. I'll be more careful to include comments
> > > > in future.
> > > >
> > > > Any other useful conventions out there for marking thread safe/unsafe
> > > > functions? I think the public == thread safe is generally a good rule
> > > > but I've yet to find a satisfactory way of marking private thread-unsafe
> > > > functions as such.
> > >
> > > Generally I just put a comment (in header file or both) saying that a
> > > prerequisite for this function is to be holding a particular lock.
> > >
> > > I think "the correct" way to do this is to use something like
> > > "assert(<lock held>);". I think this is the correct way as I think you
> > > should document (as many as possible) preconditions with asserts.
> > > However we don't have a way of expressing <lock held> efficiently as far
> > > as I know.
> >
> > The point of the signature change (or Alexandrescus use of volatile) is
> > to catch errors at compile time rather than run time. Asserts and
> > comments don't help with that. The idea of the dummy ScopedLock&
> > parameter is to make it impossible to call the function unless you have
> > a local ScopedLock or were passed a ScopedLock&, so it's more difficult
> > to accidentally call an unlocked function when you don't hold the lock.
>
> I understand this. To me the assert approach is easy to follow as it is
> self documenting (you are explicitly asserting that a specific lock is
> held), even if it isn't at compile time. The conventions approach -
> yours and the volatile approach - require some knowledge external to the
> code to understand.
>
> I thought the "volatile" approach had some currency and use, and so
> would be understood - you've demonstrated that isn't true. Your approach
> unfortunately isn't self documenting and makes the code look odd to my
> eyes
Agreed that is a problem. In the past I've also used naming conventions
like fooUnlocked(). I still don't have any solution that I really like.
> - I'm willing to go along with it, I just happen to think that
> using assert() is better in this case - especially as we have no low
> cost way to do the assertion in any case.
Confused me with that last sentence - the first part says we should use
assert(), but the second part says we can't?
RE: [c++] Experience with Alexandrescu's use of "volatile" toenforce locking.
Posted by Steve Huston <sh...@riverace.com>.
> On Mon, 2008-10-06 at 14:23 -0400, Alan Conway wrote:
> > On Mon, 2008-10-06 at 13:39 -0400, Andrew Stitcher wrote:
> > > On Mon, 2008-10-06 at 13:29 -0400, Alan Conway wrote:
> > > > On Mon, 2008-10-06 at 11:38 -0400, Andrew Stitcher wrote:
> > > > > On Fri, 2008-10-03 at 17:31 -0400, Alan Conway wrote:
> > > > > > ...
> > > > > > (1) my usual style being:
> > > > > > - all public member functions are thread safe.
> > > > > > - use ScopedLock to acquire/release locks.
> > > > > > - private, unlocked functions that are only
> intended to be called with
> > > > > > the lock already held take a dummy extra paramater
> of type ScopedLock&
> > > > > >
> > > > > > The dummy ScopedLock& parameter marks functions
> that are not locked and
> > > > > > also makes it hard to accidentally call them in an
> unlocked context.
> > > > >
> > > > > This would explain why I occasionally see odd
> functions with an unused
> > > > > ScopedLock& parameter, and try and figure out why the
> person who wrote
> > > > > it did that. Then sigh, and go and remove the useless
> parameter before
> > > > > checking in my changes.
> > > > >
> > > > > Did I miss a wiki note/or an email? Oh, such is life.
> > > >
> > > > I generally stick a comment in the .h file but very
> possible I've
> > > > forgotten on occasion. I sent round an email when I
> first started doing
> > > > this, but that's easy to miss. I'll be more careful to
> include comments
> > > > in future.
> > > >
> > > > Any other useful conventions out there for marking
> thread safe/unsafe
> > > > functions? I think the public == thread safe is
> generally a good rule
> > > > but I've yet to find a satisfactory way of marking
> private thread-unsafe
> > > > functions as such.
> > >
> > > Generally I just put a comment (in header file or both)
> saying that a
> > > prerequisite for this function is to be holding a particular
lock.
> > >
> > > I think "the correct" way to do this is to use something like
> > > "assert(<lock held>);". I think this is the correct way
> as I think you
> > > should document (as many as possible) preconditions with
asserts.
> > > However we don't have a way of expressing <lock held>
> efficiently as far
> > > as I know.
> >
> > The point of the signature change (or Alexandrescus use of
> volatile) is
> > to catch errors at compile time rather than run time. Asserts and
> > comments don't help with that. The idea of the dummy ScopedLock&
> > parameter is to make it impossible to call the function
> unless you have
> > a local ScopedLock or were passed a ScopedLock&, so it's
> more difficult
> > to accidentally call an unlocked function when you don't
> hold the lock.
>
> I understand this. To me the assert approach is easy to
> follow as it is
> self documenting (you are explicitly asserting that a specific lock
is
> held), even if it isn't at compile time. The conventions approach -
> yours and the volatile approach - require some knowledge
> external to the
> code to understand.
>
> I thought the "volatile" approach had some currency and use, and so
> would be understood - you've demonstrated that isn't true.
> Your approach
> unfortunately isn't self documenting and makes the code look odd to
my
> eyes - I'm willing to go along with it, I just happen to think that
> using assert() is better in this case - especially as we have no low
> cost way to do the assertion in any case.
In my experience, these approaches both have their benefits and can be
used together. Putting the requirement in the method signature can be
documenting as long as the author documents it - the benefit of having
the compiler find the problem is huge and, IMO, overrides almost any
other argument against it.
Asserting inside the method is beneficial to catch erroneous
values/objects passed in at run time. The biggest problem I've seen
with this approach is that people tend to put code in the assert
that's necessary for correct execution and when the asserts get
compiled to no-ops, so does the required code. Thus, you build and
test and things are great, then you build a "release" mode and the
tests start failing on odd ways that you can't find with the debugger.
All in all, I favor compile-time checks.
-Steve
Re: [c++] Experience with Alexandrescu's use of "volatile" to
enforce locking.
Posted by Andrew Stitcher <as...@redhat.com>.
On Mon, 2008-10-06 at 14:23 -0400, Alan Conway wrote:
> On Mon, 2008-10-06 at 13:39 -0400, Andrew Stitcher wrote:
> > On Mon, 2008-10-06 at 13:29 -0400, Alan Conway wrote:
> > > On Mon, 2008-10-06 at 11:38 -0400, Andrew Stitcher wrote:
> > > > On Fri, 2008-10-03 at 17:31 -0400, Alan Conway wrote:
> > > > > ...
> > > > > (1) my usual style being:
> > > > > - all public member functions are thread safe.
> > > > > - use ScopedLock to acquire/release locks.
> > > > > - private, unlocked functions that are only intended to be called with
> > > > > the lock already held take a dummy extra paramater of type ScopedLock&
> > > > >
> > > > > The dummy ScopedLock& parameter marks functions that are not locked and
> > > > > also makes it hard to accidentally call them in an unlocked context.
> > > >
> > > > This would explain why I occasionally see odd functions with an unused
> > > > ScopedLock& parameter, and try and figure out why the person who wrote
> > > > it did that. Then sigh, and go and remove the useless parameter before
> > > > checking in my changes.
> > > >
> > > > Did I miss a wiki note/or an email? Oh, such is life.
> > >
> > > I generally stick a comment in the .h file but very possible I've
> > > forgotten on occasion. I sent round an email when I first started doing
> > > this, but that's easy to miss. I'll be more careful to include comments
> > > in future.
> > >
> > > Any other useful conventions out there for marking thread safe/unsafe
> > > functions? I think the public == thread safe is generally a good rule
> > > but I've yet to find a satisfactory way of marking private thread-unsafe
> > > functions as such.
> >
> > Generally I just put a comment (in header file or both) saying that a
> > prerequisite for this function is to be holding a particular lock.
> >
> > I think "the correct" way to do this is to use something like
> > "assert(<lock held>);". I think this is the correct way as I think you
> > should document (as many as possible) preconditions with asserts.
> > However we don't have a way of expressing <lock held> efficiently as far
> > as I know.
>
> The point of the signature change (or Alexandrescus use of volatile) is
> to catch errors at compile time rather than run time. Asserts and
> comments don't help with that. The idea of the dummy ScopedLock&
> parameter is to make it impossible to call the function unless you have
> a local ScopedLock or were passed a ScopedLock&, so it's more difficult
> to accidentally call an unlocked function when you don't hold the lock.
I understand this. To me the assert approach is easy to follow as it is
self documenting (you are explicitly asserting that a specific lock is
held), even if it isn't at compile time. The conventions approach -
yours and the volatile approach - require some knowledge external to the
code to understand.
I thought the "volatile" approach had some currency and use, and so
would be understood - you've demonstrated that isn't true. Your approach
unfortunately isn't self documenting and makes the code look odd to my
eyes - I'm willing to go along with it, I just happen to think that
using assert() is better in this case - especially as we have no low
cost way to do the assertion in any case.
A
Re: [c++] Experience with Alexandrescu's use of "volatile" to
enforce locking.
Posted by Alan Conway <ac...@redhat.com>.
On Mon, 2008-10-06 at 13:39 -0400, Andrew Stitcher wrote:
> On Mon, 2008-10-06 at 13:29 -0400, Alan Conway wrote:
> > On Mon, 2008-10-06 at 11:38 -0400, Andrew Stitcher wrote:
> > > On Fri, 2008-10-03 at 17:31 -0400, Alan Conway wrote:
> > > > ...
> > > > (1) my usual style being:
> > > > - all public member functions are thread safe.
> > > > - use ScopedLock to acquire/release locks.
> > > > - private, unlocked functions that are only intended to be called with
> > > > the lock already held take a dummy extra paramater of type ScopedLock&
> > > >
> > > > The dummy ScopedLock& parameter marks functions that are not locked and
> > > > also makes it hard to accidentally call them in an unlocked context.
> > >
> > > This would explain why I occasionally see odd functions with an unused
> > > ScopedLock& parameter, and try and figure out why the person who wrote
> > > it did that. Then sigh, and go and remove the useless parameter before
> > > checking in my changes.
> > >
> > > Did I miss a wiki note/or an email? Oh, such is life.
> >
> > I generally stick a comment in the .h file but very possible I've
> > forgotten on occasion. I sent round an email when I first started doing
> > this, but that's easy to miss. I'll be more careful to include comments
> > in future.
> >
> > Any other useful conventions out there for marking thread safe/unsafe
> > functions? I think the public == thread safe is generally a good rule
> > but I've yet to find a satisfactory way of marking private thread-unsafe
> > functions as such.
>
> Generally I just put a comment (in header file or both) saying that a
> prerequisite for this function is to be holding a particular lock.
>
> I think "the correct" way to do this is to use something like
> "assert(<lock held>);". I think this is the correct way as I think you
> should document (as many as possible) preconditions with asserts.
> However we don't have a way of expressing <lock held> efficiently as far
> as I know.
The point of the signature change (or Alexandrescus use of volatile) is
to catch errors at compile time rather than run time. Asserts and
comments don't help with that. The idea of the dummy ScopedLock&
parameter is to make it impossible to call the function unless you have
a local ScopedLock or were passed a ScopedLock&, so it's more difficult
to accidentally call an unlocked function when you don't hold the lock.
Re: [c++] Experience with Alexandrescu's use of "volatile" to
enforce locking.
Posted by Andrew Stitcher <as...@redhat.com>.
On Mon, 2008-10-06 at 13:29 -0400, Alan Conway wrote:
> On Mon, 2008-10-06 at 11:38 -0400, Andrew Stitcher wrote:
> > On Fri, 2008-10-03 at 17:31 -0400, Alan Conway wrote:
> > > ...
> > > (1) my usual style being:
> > > - all public member functions are thread safe.
> > > - use ScopedLock to acquire/release locks.
> > > - private, unlocked functions that are only intended to be called with
> > > the lock already held take a dummy extra paramater of type ScopedLock&
> > >
> > > The dummy ScopedLock& parameter marks functions that are not locked and
> > > also makes it hard to accidentally call them in an unlocked context.
> >
> > This would explain why I occasionally see odd functions with an unused
> > ScopedLock& parameter, and try and figure out why the person who wrote
> > it did that. Then sigh, and go and remove the useless parameter before
> > checking in my changes.
> >
> > Did I miss a wiki note/or an email? Oh, such is life.
>
> I generally stick a comment in the .h file but very possible I've
> forgotten on occasion. I sent round an email when I first started doing
> this, but that's easy to miss. I'll be more careful to include comments
> in future.
>
> Any other useful conventions out there for marking thread safe/unsafe
> functions? I think the public == thread safe is generally a good rule
> but I've yet to find a satisfactory way of marking private thread-unsafe
> functions as such.
Generally I just put a comment (in header file or both) saying that a
prerequisite for this function is to be holding a particular lock.
I think "the correct" way to do this is to use something like
"assert(<lock held>);". I think this is the correct way as I think you
should document (as many as possible) preconditions with asserts.
However we don't have a way of expressing <lock held> efficiently as far
as I know.
Andrew
>
Re: [c++] Experience with Alexandrescu's use of "volatile" to
enforce locking.
Posted by Alan Conway <ac...@redhat.com>.
On Mon, 2008-10-06 at 11:38 -0400, Andrew Stitcher wrote:
> On Fri, 2008-10-03 at 17:31 -0400, Alan Conway wrote:
> > ...
> > (1) my usual style being:
> > - all public member functions are thread safe.
> > - use ScopedLock to acquire/release locks.
> > - private, unlocked functions that are only intended to be called with
> > the lock already held take a dummy extra paramater of type ScopedLock&
> >
> > The dummy ScopedLock& parameter marks functions that are not locked and
> > also makes it hard to accidentally call them in an unlocked context.
>
> This would explain why I occasionally see odd functions with an unused
> ScopedLock& parameter, and try and figure out why the person who wrote
> it did that. Then sigh, and go and remove the useless parameter before
> checking in my changes.
>
> Did I miss a wiki note/or an email? Oh, such is life.
I generally stick a comment in the .h file but very possible I've
forgotten on occasion. I sent round an email when I first started doing
this, but that's easy to miss. I'll be more careful to include comments
in future.
Any other useful conventions out there for marking thread safe/unsafe
functions? I think the public == thread safe is generally a good rule
but I've yet to find a satisfactory way of marking private thread-unsafe
functions as such.