Implementing a simple registry watcher by keith-horton · Pull Request #365 · microsoft/wil

keith-horton · 2023-09-26T06:22:19Z

Implementing a simple registry watcher, which maintains the same contract as the unique_threadpool objects it uses for the callbacks - it guarantees all callbacks have completed and no more will be invoked when the d'tor has completed

…ract as the unique_threadpool objects it uses for the callbacks - it guarantees all callbacks have completed and no more will be invoked when the d'tor has completed

dmachaj · 2023-09-26T17:02:33Z

include/wil/registry.h

+        slim_registry_watcher_t() WI_NOEXCEPT = default;
+
+        // Exception-based constructors
+        slim_registry_watcher_t(HKEY rootKey, _In_ PCWSTR subKey, bool isRecursive, ::wistd::function<void(::wil::RegistryChangeKind)>&& callback)


Perhaps it would be safer if there were a series of constructors that took various kinds of smart pointers (weak and strong). The registry watcher can then keep the callback alive (if given a strong pointer) or allow it to safely destruct (if given a weak pointer).

I'm at a loss to see what benefit differing behaviors provides based off the type of callback one provides (well, what type of container holds their callback).

It seems some folks really wanted to decouple the object enabling the callbacks from their callback code that runs. That doesn't make sense to me - if I want to unregister, I don't want more callbacks. But I'm only 1 person, so cool - if some folks want to employ weak-ref models with their callback designs, and they don't want to worry about properly synchronizing when objects are destroyed -- then they have an option.

But I think that should be a strong interface contract at the class level, not implicit behavior on what type of callback container they provide.

Hope this makes sense (?)

I don't think I expressed my suggestion well. What I'm trying to suggest is that slim_registry_watcher is a tuple containing everything you already have AND some sort of lifetime management for the thing that is being called back. That could be a strong reference, a weak reference, or for non-class-based code it can be left off.

Providing a strong or weak pointer should completely close the race with object destruction. A strong reference prevents destruction. A weak reference allows the callback to know that it is already destructed so it can NOOP.

ChrisGuzak · 2023-09-26T15:53:44Z

include/wil/registry.h

    /// @endcond

+    template <typename err_policy = ::wil::err_exception_policy>
+    class slim_registry_watcher_t


one feature that is missing from the registry watcher is access to the HKEY that it holds.

if you could add that feature it would be nice.

ChrisGuzak · 2023-09-26T15:54:23Z

include/wil/registry.h

+    {
+    public:
+        // HRESULT or void error handling
+        typedef typename err_policy::result result;


nit, but here using result = err_policy::result; is the more modern syntax.

ChrisGuzak · 2023-09-26T15:54:56Z

include/wil/registry.h

-        return unique_registry_watcher_failfast(rootKey, subKey, isRecursive, wistd::move(callback));
+        return ::wil::unique_registry_watcher_failfast(rootKey, subKey, isRecursive, wistd::move(callback));
+    }
+    inline ::wil::unique_slim_registry_watcher_failfast make_slim_registry_watcher_failfast(HKEY rootKey, _In_ PCWSTR subKey, bool isRecursive, wistd::function<void(RegistryChangeKind)>&& callback)


nit, add a blank line

ChrisGuzak · 2023-09-26T15:55:05Z

include/wil/registry.h

-        return unique_registry_watcher_failfast(wistd::move(keyToWatch), isRecursive, wistd::move(callback));
+        return ::wil::unique_registry_watcher_failfast(wistd::move(keyToWatch), isRecursive, wistd::move(callback));
+    }
+    inline ::wil::unique_slim_registry_watcher_failfast make_slim_registry_watcher_failfast(unique_hkey&& keyToWatch, bool isRecursive, wistd::function<void(RegistryChangeKind)>&& callback)


nit, add blank line

ChrisGuzak · 2023-09-26T15:56:20Z

tests/RegistryTests.cpp

+    SECTION("unique_slim_registry_watcher_nothrow with recurssive changes")
+    {
+        wil::unique_hkey hkey;
+        REQUIRE_SUCCEEDED(wil::reg::create_unique_key_nothrow(HKEY_CURRENT_USER, testSubkey, hkey, wil::reg::key_access::readwrite));


consider using the throwing version to simplify the test and avoid treating this setup code as the test subject. that is avoid using REQUIRE_XXX on things other than the test subject (your slim reg watcher)

ChrisGuzak · 2023-09-26T15:58:53Z

tests/RegistryTests.cpp

+        {
+            callbackTracking.ResetEvent();
+            REQUIRE_SUCCEEDED(wil::reg::set_value_nothrow(hkey.get(), L"test", count));
+            REQUIRE(callbackTracking.wait(500));


test should be fast, avoid Sleep. use witest::RunUntilTrue to probe for the desired state and finish as soon as it is achieved.

ChrisGuzak · 2023-09-26T16:00:07Z

tests/RegistryTests.cpp

+            {
+                callbackEntered.SetEvent();
+                // now wait 5 seconds - ensuring we are in the d'tor on the main thread
+                Sleep(5000);


witest::RunUntilTrue

ChrisGuzak · 2023-09-26T16:03:45Z

include/wil/registry.h

+        ::wil::unique_threadpool_wait m_threadPoolWait;
+        bool m_isRecursive;
+
+        static void __stdcall callback(PTP_CALLBACK_INSTANCE, void* context, TP_WAIT*, TP_WAIT_RESULT) WI_NOEXCEPT


to enable testing the races, you can add a template param used in a constexpr expression that lets you control a delay value. this is a NOP in retail, but your test version has a call to Sleep().

This is how I tested the races with COM apartment variables. see this example

ChrisGuzak · 2023-09-26T16:07:18Z

tests/RegistryTests.cpp

        REQUIRE(count == 4);
    }
 }
+TEST_CASE("BasicRegistryTests::slim_registry_watcher_t", "[registry]]")


a tricky case that I struggled with when creating the registry watcher was the ability to call reset in the callback. Maybe that is the source of the bug. here is the test for that.

but make sure you can tolerate that case too.

ChrisGuzak · 2023-09-26T16:09:56Z

include/wil/registry.h

    /// @endcond

+    template <typename err_policy = ::wil::err_exception_policy>
+    class slim_registry_watcher_t


write a comment that explains the relationship between this new imlp and the existing.

I assume we want all uses of registry_watcher to be replaced with this one. If so maybe safe_registry_watcher is a better name. or registry_watcher2 to make it clear it is a superset.

Implementing a simple registry watcher, which maintains the same cont…

667a2e7

…ract as the unique_threadpool objects it uses for the callbacks - it guarantees all callbacks have completed and no more will be invoked when the d'tor has completed

dmachaj reviewed Sep 26, 2023

View reviewed changes

ChrisGuzak reviewed Sep 26, 2023

View reviewed changes

Merge branch 'master' into user/khorton/simple_registry_watcher

e93e619

Conversation

keith-horton commented Sep 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants