An Issue I Wish To Raise Awareness On

[Jonathan M Davis via Digitalmars-d]

On Wednesday, July 19, 2017 8:59:03 PM MDT Atila Neves via Digitalmars-d 
wrote:
> On Wednesday, 19 July 2017 at 20:23:18 UTC, Jonathan M Davis
>
> wrote:
> > On Wednesday, July 19, 2017 2:29:04 PM MDT Atila Neves via
> >
> > Digitalmars-d wrote:
> >> On Tuesday, 18 July 2017 at 19:24:18 UTC, Jonathan M Davis
> >>
> >> wrote:
> >> > On Tuesday, July 18, 2017 18:06:56 Atila Neves via
> >> >
> >> >> Except for a programmer explicitly and manually calling the
> >> >> destructor (in which case, don't), the destructor is only
> >> >> ever called by one thread.
> >> >
> >> > It could still be a problem if the struct has a member
> >> > variable that is a reference type, because then something
> >> > else could refer to that object, and if it's shared, then
> >> > you would need to protect it, and the operations that shared
> >> > prevents should still be prevented. For full-on value types,
> >> > it should be a non-issue though.
> >> >
> >> > - Jonathan M Davis
> >>
> >> Mmm, I guess so. As Marco pointed out, it's a similar problem
> >> with immutable (because the compiler casts it away before
> >> calling the destructor).
> >>
> >> Although I dare say that anybody writing code that depends on
> >> such locking and destruction when shared is unlikely to get it
> >> right in the first place.
> >
> > Well, consider that something like a reference counted type
> > would have to get this right. Now, a reference counted type
> > that worled with shared would need to be written that way -
> > simply slapping shared on such a smart pointer type isn't going
> > to work - but it would then be really bad for the destructor to
> > be treated as thread-local.
>
> Not necessarily - the reference counted smart pointer doesn't
> have to be `shared` itself to have a `shared` payload. I'm not
> even entirely sure what the advantage of it being `shared` would
> be, or even what that would really mean.
>
> The way `automem.RefCounted` works right now is by doing atomic
> reference count manipulations by using reflection to know if the
> payload is `shared`.

Okay, but my point is that it's perfectly legal to have a shared object that
refers to other shared objects that it does not own, and casting away shared
in the destructor means that the compiler can no longer enforce shared like
it's supposed to.

> > It really looks to me like having a thread-local dstructor for
> > shared data is just begging for problems. It may work in the
> > simple cases, but it'll fall apart in the more complicated
> > ones, and I don't see how that's acceptable.
>
> You've definitely made me wonder about complicated cases, but I'd
> argue that they'd be rare. Destructors are (bar manually calling
> them) run in one thread. I'm having trouble imagining a situation
> where two threads have references to a `shared` object/value that
> is going to be destroyed deterministically.

The issue isn't the object being destroyed. It's what it refers to via its
member variables. For instance, what if an object were to remove itself from
a shared list when it's destroyed (e.g. because it's an observer in the
observer pattern). The object has a reference to the list, but it doesn't
own it. So, the fact that the destructor is only run in a single thread
doesn't help any. You could have ten of these objects all being destroyed
and accessing the same list at the same time from different threads, and if
the destructor treats the objects as thread-local - and thus treats all of
the member variables as thread-local, then you won't get the compiler errors
that you're supposed to get when you do something non-atomic with shared.

Sure, the list could be designed in such a way that it protects itself
against threading issues, but it's perfectly legal to have it be shared and
require that anyone using it lock a mutex, and destructors need to take that
into account just like any other function would.

Basically, what these changes have done is act like all destructors are part
of a synchronized class (as described in TDPL) where shared is stripped off
of the member variables - except that _all_ of shared has been stripped off
instead of just the outer layer, but these aren't synchronized classes, and
they don't provide any guarantees about references not escaping or locking
occurring, and not even synchronized classes can safely cast away shared
except for the outer layer. As such, from what I can tell, these changes are
very broken. Yes, we need a solution which allows us to deal with shared
destructors properly, but casting away shared without any guarantees beyond
the fact that no other threads are calling any member functions on that
object at that time simply does not guarantee that shared objects have been
properly protected. To do that, you'd essentially need synchronized classes,
and even they can only strip off the outer layer of shared, not the whole
thing. Yes, destructors have fewer problems with shared than most functions,
but there's nothing about then which makes them immune to issues with
shared.

- Jonathan M Davis