5 ms·

Their Synchronized<> is a thing we also came up with to use in our code. It's so helpful it should be part of the Standard Library.

secondcoming 5y ago
It looks like part of it is a similar design to something I came up with myself. Do you know if Synchronized<> protects against accidental recursive locking?
- ot 5y ago
  There is no explicit protection for recursive locking. In practice, write locks will deadlock, and read lock under read lock will be fine. However, the API encourages practices that make it very hard to recursively acquire the lock (Synchronized is about access to objects, not critical sections).
jimsimmons 5y ago
What does it do
jemfinch 5y ago
Having reviewed even just a few diffs with Synchronized since joining Facebook, it absolutely should not be part of the standard library (nor, I would argue, part of folly). I have found far too many cases where engineers have threading issues in their code because they're saving references to objects after .lock() has returned. Writing thread-safe code isn't easy, and it isn't made easier by libraries that offer footguns like Synchronized. It's easier to get it right with compiler thread annotations than with Synchronized.
- ot 5y ago
  How do compiler thread annotations solve the problem of persisting references to the object? Is static analysis able to detect that? Seems a hard problem (basically escape analysis). Synchronized cannot guarantee thread-safety, but in my experience wrappers that do not allow unsynchronized access to the object are better than the alternative. Annotations are just another way to guard access.
  - jemfinch 5y ago
    They don't, but C++ programmers especially are familiar with not letting inappropriate things escape scopes; they're much less familiar with not letting inappropriate things escape _expressions_. I've reviewed over a half million lines of code from engineers of every level of experience, and I've seen far more bugs from the latter than the former. For instance, a former employer had a function similar to folly::get_default, except it returned the default by reference, and engineers were always running afoul of lifetimes using that function. (It was especially egregious with std::string temporaries returned from string literal defaults.) Thread annotations also effectively prevent unsynchronized access to the object, but they do it in a way that programmers make fewer mistakes with, and they do it with a less error prone mental model: mutexes are seen as guards of _class invariants_, not merely as guards of _data members_. Apart from encouraging a superior mental model, thread annotations also encourage a one-mutex-per-class model which I've found far more successful and error prone (both in code I've reviewed and in code I've written) over a one-mutex-per-member model which inevitably (in my experience) results in fewer bugs.

secondcoming 5y ago

It looks like part of it is a similar design to something I came up with myself. Do you know if Synchronized<> protects against accidental recursive locking?

ot 5y ago
There is no explicit protection for recursive locking. In practice, write locks will deadlock, and read lock under read lock will be fine. However, the API encourages practices that make it very hard to recursively acquire the lock (Synchronized is about access to objects, not critical sections).

jimsimmons 5y ago

What does it do

jemfinch 5y ago

Having reviewed even just a few diffs with Synchronized since joining Facebook, it absolutely should not be part of the standard library (nor, I would argue, part of folly). I have found far too many cases where engineers have threading issues in their code because they're saving references to objects after .lock() has returned. Writing thread-safe code isn't easy, and it isn't made easier by libraries that offer footguns like Synchronized. It's easier to get it right with compiler thread annotations than with Synchronized.

ot 5y ago
How do compiler thread annotations solve the problem of persisting references to the object? Is static analysis able to detect that? Seems a hard problem (basically escape analysis). Synchronized cannot guarantee thread-safety, but in my experience wrappers that do not allow unsynchronized access to the object are better than the alternative. Annotations are just another way to guard access.
- jemfinch 5y ago
  They don't, but C++ programmers especially are familiar with not letting inappropriate things escape scopes; they're much less familiar with not letting inappropriate things escape _expressions_. I've reviewed over a half million lines of code from engineers of every level of experience, and I've seen far more bugs from the latter than the former. For instance, a former employer had a function similar to folly::get_default, except it returned the default by reference, and engineers were always running afoul of lifetimes using that function. (It was especially egregious with std::string temporaries returned from string literal defaults.) Thread annotations also effectively prevent unsynchronized access to the object, but they do it in a way that programmers make fewer mistakes with, and they do it with a less error prone mental model: mutexes are seen as guards of _class invariants_, not merely as guards of _data members_. Apart from encouraging a superior mental model, thread annotations also encourage a one-mutex-per-class model which I've found far more successful and error prone (both in code I've reviewed and in code I've written) over a one-mutex-per-member model which inevitably (in my experience) results in fewer bugs.