Hacker News Reader

Comment by cjensen | Hacker News Reader

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.

View on HN

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.

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.

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.

What does it do

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?

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).

Reply on news.ycombinator.com