On Tue, Apr 01, 2014 at 08:34:01PM -0400, Ehsan Akhgari wrote:
> On 2014-04-01, 7:58 PM, Mike Hommey wrote:
> >On Tue, Apr 01, 2014 at 05:32:12PM -0400, Ehsan Akhgari wrote:
> >>The subject of this post is intentionally chosen to make you want to read
> >>this. :-)
> >>
> >>The summary is that I think the mozilla::Atomic API which is modeled after
> >>std::atomic is harmful in that it makes it very non-obvious that you're
> >>dealing with atomic integers. Basically the existing interface makes
> >>mozilla::Atomic look like a normal integer, so that if you see code like:
> >>
> >>--mRefCnt;
> >>if (mRefCnt == 0) {
> >> delete this;
> >>}
> >>
> >>You won't immediately think about checking the type of mRefCnt (the
> >>refcount case being just an example here of course), which makes it hard to
> >>spot that there is a thread safety bug in this code.
> >
> >Actually, the thread safety bug in this code is largely the same whether the
> >type of mRefCnt is mozilla::Atomic or not. Compiler optimizations may
> >remove the bug, but it's there to begin with.
>
> That is the code after I changed it. :-) Here is the original patch which
> introduced this bug:
> <https://bug935778.bugzilla.mozilla.org/attachment.cgi?id=8385072> The
> thread safety issue was _not_ there before that patch.
I'm not saying there was a thread safety issue before. I'm saying that in
your example, there is largely the same thread safety issue whether the
code uses a plain int or an atomic one. The use of atomics is _not_ hiding
this bug.
> >As I said in the bug, all this is saying is that thread safety is hard,
> >and atomics are merely one of the tools to achieve thread safety. They
> >are not a magic wand that fixes thread safety.
>
> Did you read <https://bugzilla.mozilla.org/show_bug.cgi?id=987887#c20>?
Did you read my answer to that comment?
> Continuing to reduce my argument here to "atomics should be a magic wand" is
> really depressing.
>
> >I also think that making their API not have operators like std::atomic
> >has would bring a false sense of security to people writing code using
> >them, because it would supposedly be less confusing when it really
> >wouldn't.
>
> Can you please give an example of that concern? I'm not sure if I follow.
Just take the original patch which introduced the bug. I seriously don't
believe if the patch had been using FetchAndAdd instead of ++/-- the issue
would have had more chance of being caught at review. And I don't
believe it would make people more likely to make atomic-using code
thread safe.
Back to Kyle's comment from the bug, the core issue is that we don't
have anything that tells us that $function is expected to be called from
different threads at the same time, and that as such it _needs_ to be
thread safe or reentrant in some way. Maybe we need to invent those
markers, and to have ways to test when they are missing (like special
builds that ensure that functions without those markers are never called
from two threads simultaneously).
Mike
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
