5 ms·

Neat idea. Though I think having to pass a lambda for anything you want to do with the fields is awful ergonomics. Maybe instead combine the two ideas. MutexPr

• akling 4y ago
  Yes, that's a great way to improve the ergonomics of MutexProtected in simple cases! We should totally add that as a complement to the lambda API. :)
  • akling 4y ago
    After sketching this out, it's nice that this: auto x = state.with([](auto& state) { return state.x; }); Becomes this: auto x = state.locked()->x; But it also creates this very accessible footgun: auto& x = state.locked()->x; Where it's way too easy to bind a reference to something that should be protected by the lock, but now isn't. So I'm not sure this is a great idea anymore.
    • gpderetta 4y ago
      At the end of the day, this is C++, there are no compiler tracked lifetimes and it is easy to leak references out of the lambda as well. At least the synchronized pattern makes it easy to document which state is protected by which mutex.
    • kajaktum 4y ago
      Wait why is this a footgun?
      • akling 4y ago
        Because now you're holding a reference to `x` which is supposed to be protected by a mutex, even after the mutex is unlocked. With the lambda-only API, it's much harder to make this mistake, since a temporary reference like this will still go out of scope at the end of the lambda expression.
        • kajaktum 4y ago
          You specifically mentioned that this is a footgun: > auto& x = state.locked()->x; But I don't see how the reference here is gonna make a difference unless i am reading the lifetime of the lock here incorrectly. For example, this is perfectly fine right? ``` { auto& x = state.locked()->x; } ``` This will only be a problem if you have an outside struct that holds a reference ``` auto &a = "something"; { auto& x = state.locked()->x; a = x; } ``` Which can still happen even if you use a lambda.
      • MontagFTB 4y ago
        Holding the reference to a field that is protected by a mutex implies there is another thread out there that will race with your reference in either reading or writing it. Even just a read is racy, as there is no “atomic read” of any size value if it is not already wrapped as atomic.
    • Blackthorn 4y ago
      > But it also creates this very accessible footgun: Well, you said it yourself in the article though. There's always ways around the locking; C++ doesn't really give you the full ability to guarantee a field is locked when access. You'll need to either trust the users to some extent or use a style guide to disallow the pattern (I'd suggest only allowing use of auto x = state.locked() to avoid lifetime questions around state.locked()->x). You'd need to use compiler annotations to get any better.
• secondcoming 4y ago
  That looks even worse than a simple callback. Does it preserve the correct ‘constness’ of the field in the case of shared mutexes?
• ot 4y ago
  That's how folly::Synchronized works, it supports both the callback interface as MutexProtected, and an interface that returns a RAII lock holder that gives access to the underlying object. It is generally the preferred synchronization utility at Meta and thus it is widely used. https://github.com/facebook/folly/blob/main/folly/docs/Synchronized.md https://github.com/facebook/folly/blob/main/folly/docs/Synch...
  • biesnecker 4y ago
    As someone who had to occasionally write C++ in FBCode but not enough to really feel comfortable with it, `folly::Synchronized` and its straightforward semantics were really nice.
• mchicken 4y ago
  You guys like to reinvent Boost https://www.boost.org/doc/libs/1_81_0/doc/html/thread/sds.html#thread.sds.synchronized_valuesxxx https://www.boost.org/doc/libs/1_81_0/doc/html/thread/sds.ht....
  • FpUser 4y ago
    No, but Boost is one giant bear of dependency with lots of duplication with STL. Personally I prefer to use tiny includable file / lib for specific functionality that I need rather than keep that bear on my real estate.
    • planede 4y ago
      Boost is multiple libraries, you don't have to use all of it. Many of it are header-only as well.
      • FpUser 4y ago
        Seriously I have no desire installing this giant thing and deciphering what can be extracted and how. This is not to diminish value of Boost but it is just simply not for me.
        • misnome 4y ago
          not to mention that the documentation is generally utterly awful, with mostly only reference-style function documentation without exposition but manually distributed across lots of small 1-page paragraphs.
          • lenkite 4y ago
            This is in general is a problem with C++ libraries because doc generation is not standardized unlike Rust and other languages.
            • misnome 4y ago
• dietr1ch 4y ago
  When using locks I'm more paranoid about misusing the lock and don't mind typing a bit more to make the code obvious. Also, it seems that good auto-completion for that pattern is simple to achieve. Alternatives to make sure you are not grabbing the wrong lock include this much uglier GUARDED_BY macro, - http://clang.llvm.org/docs/ThreadSafetyAnalysis.html http://clang.llvm.org/docs/ThreadSafetyAnalysis.html I'd say the extra lambda is a fair price to pay
  • Someone 4y ago
    It’s a fair price to pay, but I think the suggested change is better because it makes you pay less for the same benefit. It still would be obvious that there’s a lock, but the code would IMO be simpler: thing->field.with([&](Field& field) { use(field); }); would become { auto field = Mutex.locker(thing.field); // or thing.field.lock(); use(field); } or use(Mutex.locker(thing.field)); Yes, that requires you to understand destructors exist, but if you’re programming C++, that’s a given. I find that easier to understand (partly because the first doesn’t mention ‘lock’. I don’t think ‘with’ is the best name there)
    • simplotek 4y ago
      > because it makes you pay less for the same benefit. Requiring a lambda capturing stuff and be passed around is not what I would call "pay less", when the alternative is just adding a block and instantiate a std::lock_guard
      • Someone 4y ago
        Fuller quote: “I think the suggested change is better because it makes you pay less for the same benefit” The change suggested in this part of this discussion is by user “Blackthorn” in https://news.ycombinator.com/item?id=35464828 https://news.ycombinator.com/item?id=35464828. It says: > Neat idea. Though I think having to pass a lambda for anything you want to do with the fields is awful ergonomics. > Maybe instead combine the two ideas. MutexProtected<T> but can be locked with MutexProtected<T>::lock() which returns a MutexLocked<T> object. That object then cleans up the lock when it goes out of scope, and also provides direct access to the enclosed type. User “dietr1ch” replied in https://news.ycombinator.com/item?id=35465610 https://news.ycombinator.com/item?id=35465610 he wanted to pay with a lambda to make it clear a lock is taken. I tried to clarify that Blackthorn‘s suggestion would make that clear without requiring that lambda.
• somerando7 4y ago
  IMO passing a lambda for synchronized code makes it much easier to read (going off of working with folly::Synchronized)
• avidphantasm 4y ago
  Yeah, the syntax/ergonomics is tortured. This is my problem with modern C++ in general. As someone who isn’t a full-time C++ programmer (but who maintains a library written in C++), I have to shy away from anything too fancy as the syntax is too baroque and hard to recall.
  • simplotek 4y ago
    > Yeah, the syntax/ergonomics is tortured. This is my problem with modern C++ in general. I disagree. I mean, the ergonomics in this case are indeed awful, but this has nothing to do with C++, modern or not. This is an API design problem, possibly made worse by people trying to be too clever for their own good.
• simplotek 4y ago
  > Neat idea. I don't agree. It sounds like a higher level path to deadlocks. > Maybe instead combine the two ideas. MutexProtected<T> but can be locked with MutexProtected<T>::lock() which returns a MutexLocked<T> object. That object then cleans up the lock when it goes out of scope, and also provides direct access to the enclosed type. It sounds like you're trying to invent std::lock_guard with extra steps. https://en.cppreference.com/w/cpp/thread/lock_guard https://en.cppreference.com/w/cpp/thread/lock_guard
  • jmull 4y ago
    > It sounds like ... std::lock_guard with extra steps. I'm not the previous poster, but I think they are still suggesting that the value not be accessible until the lock is acquired. Something like this (Apologies, I haven't done C++ is a long time and this is just off the top of my head. It's based on the example from the lock_guard example for the sake of comparison): guardthing<int> guarded_value; void safe_increment() { auto lock = guarded_value.lock(); lock.value += 1; std::cout << "value: " << value << "; in thread #" << std::this_thread::get_id() << '\n'; // the mutex in guarded_value is automatically released when lock // goes out of scope }
  • Blackthorn 4y ago
    > It sounds like you're trying to invent std::lock_guard with extra steps. No, because lock_guard doesn't guard fields. lock_guard is simply the RAII option they were talking about in the article.
• loeg 4y ago
  > MutexProtected<T> but can be locked with MutexProtected<T>::lock() which returns a MutexLocked<T> object. That object then cleans up the lock when it goes out of scope, and also provides direct access to the enclosed type. In fact, MutexProtected provides ::lock_exclusive() and ::lock_shared() methods which do exactly that. The article just fails to mention them. https://github.com/SerenityOS/serenity/blob/master/Kernel/Locking/MutexProtected.h#L58-L59 https://github.com/SerenityOS/serenity/blob/master/Kernel/Lo...