This patch introduces a runUnlocked() method in WTF::DataMutex::LockedWrapper to run a lambda function without the lock. This is intended to be used for small sections of the code that need to be unlocked, in cases where using scoping would prove non-ergonomic or where running the unlocked section is only necessary or desired when a certain condition is met -- something that cannot be done with C++ scoping. Safety mechanisms are provided. First, because this is used with a lambda, all variables to be used in the unlocked section have to be specified in the capture (global capture is possible but not recommended to simplify analysis). Second, additional checks have been added to DataMutex to detect unlocked accesses among other conditions. This will detect among other things naive access to protected members by means of capturing the LockedWrapper by reference.
Created attachment 395042 [details] Patch
Comment on attachment 395042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395042&action=review > Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp:30 > +#define ENABLE_DATA_MUTEX_CHECKS 1 :) > Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp:52 > + wrapper.runUnlocked([mutex]() { I don't have strong feelings here, but runUnlocked seems like it could encourage a smell. Locked scopes with lots of little unlock/lock pairs are nasty. Hard to say because I don't see real cases in your patch. wrapper.runUnlockedIf(condition, block) could be useful as well per your ChangeLog.
Comment on attachment 395042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395042&action=review >> Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp:52 >> + wrapper.runUnlocked([mutex]() { > > I don't have strong feelings here, but runUnlocked seems like it could encourage a smell. Locked scopes with lots of little unlock/lock pairs are nasty. Hard to say because I don't see real cases in your patch. > > wrapper.runUnlockedIf(condition, block) could be useful as well per your ChangeLog. The need arises. The point is to make the smell noticeable and easily auditable. Because of the lambda capture, it's easy to see what data is used during the unlocked section. And there are still checks to avoid the most obvious accidental abuses. runUnlockedIf() sounds like a useful shorthand, but in practice, it's not more ergonomical. The reason for that is that very often after returning from the unlocked section you'll need to check that you still need to proceed with the original code (think GStreamer flushes for example). Here is an example of an usage of runUnlocked() in WebKitWebSrc create() function, so you get an idea of the scenarios where it comes handy: if (members->pendingCaps) { GST_DEBUG_OBJECT(src, "Setting caps: %" GST_PTR_FORMAT, members->pendingCaps.get()); members.runUnlocked([&baseSrc, caps = members->pendingCaps.leakRef()]() { gst_base_src_set_caps(baseSrc, caps); }); if (members->isFlushing) return GST_FLOW_FLUSHING; }
(In reply to Alicia Boya García from comment #3) > Here is an example of an usage of runUnlocked() in WebKitWebSrc create() > function, so you get an idea of the scenarios where it comes handy: Makes sense, thanks. Informal r+ from me.
Committed r259879: <https://trac.webkit.org/changeset/259879> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395042 [details].
<rdar://problem/61588719>