Bug 209811 – [WTF] DataMutex: Add runUnlocked()

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Alicia Boya García]

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.

[Alicia Boya García]

Created attachment 395042 [details]
Patch

[Charlie Turner]

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.

[Alicia Boya García]

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;
    }

[Charlie Turner]

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

[EWS]

Committed r259879: <https://trac.webkit.org/changeset/259879>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395042 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61588719>