{"id":106866,"date":"2022-07-15T07:00:00","date_gmt":"2022-07-15T14:00:00","guid":{"rendered":"https:\/\/devblogs.microsoft.com\/oldnewthing\/?p=106866"},"modified":"2022-11-30T06:51:27","modified_gmt":"2022-11-30T14:51:27","slug":"20220715-00","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/oldnewthing\/20220715-00\/?p=106866","title":{"rendered":"Processing a ValueSet or PropertySet even in the face of possible mutation, part 4"},"content":{"rendered":"

The customer that was trying to apply the conclusions of this series had a scenario where the PropertySet had both an auto-save feature, as well as an explicit-save feature. So they tried refactoring:<\/p>\n

std::optional<SomeKindOfDataBuffer> Widget::SaveToBuffer()<\/span>\r\n{\r\n    try {\r\n        SomeKindOfDataBuffer buffer;\r\n        auto it = m_propertySet.First();\r\n        if (it.HasCurrent()) {\r\n            do {\r\n                auto current = it.Current();\r\n                buffer.AddKeyAndValue(current.Key(), current.Value());\r\n            } while (it.MoveNext());\r\n        }\r\n\r\n        auto guard = m_lock.lock();\r\n\r\n        \/\/ verify that the collection is still unchanged before returning\r\n        std::ignore = it.HasCurrent();\r\n\r\n        return buffer;<\/span>\r\n    } catch (winrt::hresult_changed_state const&) {\r\n        \/\/ Abandon the operation.\r\n        return std::nullopt;<\/span>\r\n    }\r\n}\r\n\r\nvoid Widget::OnPropertySetChanged()\r\n{\r\n    auto buffer = SaveToBuffer();\r\n    if (buffer) {\r\n        SaveToAutoSaveLocation(*buffer);\r\n    }\r\n}\r\n\r\nvoid Widget::Save()\r\n{\r\n    auto buffer = SaveToBuffer();\r\n    if (!buffer) throw winrt::hresult_changed_state();\r\n    SaveBuffer(*buffer);\r\n    return true;\r\n}<\/span>\r\n<\/pre>\n
The code to save the property set to a buffer is factored into a helper function, which we then use in the two code paths.<\/p>\n
Unfortunately, the refactoring reintroduced the bug.<\/p>\n
The purpose of the lock is to prevent a competing auto-save from racing ahead of the current auto-save. But the Save\u00adTo\u00adBuffer()<\/code> releases the lock before returning. The new auto-save code reopens the race condition:<\/p>\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n
Thread 1<\/td>\nThread 2<\/td>\n<\/tr>\n
Insert begins<\/td>\n <\/td>\n<\/tr>\n
OnPropertySetChanged begins<\/td>\n <\/td>\n<\/tr>\n
SaveToBuffer begins<\/td>\n <\/td>\n<\/tr>\n
Create the buffer<\/td>\n <\/td>\n<\/tr>\n
Enter the lock<\/td>\n <\/td>\n<\/tr>\n
Check for concurrent mutation<\/td>\n <\/td>\n<\/tr>\n
Release the lock<\/td>\n <\/td>\n<\/tr>\n
Return from SaveToBuffer<\/td>\n <\/td>\n<\/tr>\n
\u00a0<\/td>\nInsert begins<\/td>\n<\/tr>\n
\u00a0<\/td>\nOnPropertySetChanged begins<\/td>\n<\/tr>\n
\u00a0<\/td>\nSaveToBuffer begins<\/td>\n<\/tr>\n
\u00a0<\/td>\nCreate the buffer<\/td>\n<\/tr>\n
\u00a0<\/td>\nEnter the lock<\/td>\n<\/tr>\n
\u00a0<\/td>\nCheck for concurrent mutation<\/td>\n<\/tr>\n
\u00a0<\/td>\nRelease the lock<\/td>\n<\/tr>\n
\u00a0<\/td>\nReturn from SaveToBuffer<\/td>\n<\/tr>\n
\u00a0<\/td>\nSaveToAutoSaveLocation<\/td>\n<\/tr>\n
\u00a0<\/td>\nOnPropertySetChanged returns<\/td>\n<\/tr>\n
\u00a0<\/td>\nInsert returns<\/td>\n<\/tr>\n
SaveToAutoSaveLocation<\/td>\n <\/td>\n<\/tr>\n
OnPropertySetChanged returns<\/td>\n <\/td>\n<\/tr>\n
Insert returns<\/td>\n <\/td>\n<\/tr>\n<\/tbody>\n<\/table>\n
Once you release the lock, another thread can come in and mutate the collection and save the changes. And then you get around to saving the changes, and you overwrote the good changes with out-of-date ones.<\/p>\n
Fortunately, we can solve this problem by returning the iterator, too!<\/p>\n
std::pair<std::optional<SomeKindOfDataBuffer>,\r\n          winrt::Iterator<winrt::IKeyValuePair<winrt::hstring,\r\n                                               winrt::IInspectable>>><\/span>\r\nWidget::SaveToBuffer()\r\n{\r\n    SomeKindOfDataBuffer buffer;\r\n    auto it = m_propertySet.First();<\/span>\r\n    try {\r\n        if (it.HasCurrent()) {\r\n            do {\r\n                auto current = it.Current();\r\n                buffer.AddKeyAndValue(current.Key(), current.Value());\r\n            } while (it.MoveNext());\r\n        }\r\n\r\n        \/\/ auto guard = m_lock.lock();<\/span>\r\n\r\n        \/\/ verify that the collection is still unchanged before returning<\/span>\r\n        \/\/ std::ignore = it.HasCurrent();<\/span><\/span>\r\n\r\n        return { buffer, it<\/span> };\r\n    } catch (winrt::hresult_changed_state const&) {\r\n        \/\/ Abandon the operation.\r\n        return { std::nullopt, it<\/span> };\r\n    }\r\n}\r\n<\/pre>\n
The two callers can then check the validity of the iterator to determine whether or not to proceed. In the case of auto-save, the check can be performed under the lock.<\/p>\n
void Widget::OnPropertySetChanged()\r\n{\r\n    auto [buffer, it] = SaveToBuffer();\r\n    if (buffer) {\r\n        auto guard = m_lock.lock();\r\n        try {\r\n            \/\/ final check under the lock\r\n            std::ignore = it.HasCurrent();\r\n            SaveToAutoSaveLocation(*buffer);\r\n        } catch (winrt::hresult_changed_state const&) {\r\n            \/\/ Abandon the operation.\r\n        }\r\n    }\r\n}\r\n\r\nvoid Widget::Save()\r\n{\r\n    auto [buffer, it] = SaveToBuffer();\r\n\r\n    \/\/ Verify that the collection wasn't changed\r\n    \/\/ while we were trying to build the buffer.\r\n    std::ignore = it.HasCurrent();\r\n\r\n    SaveBuffer(*buffer);\r\n}\r\n<\/pre>\n
An alternative is to make the caller provide the iterator:<\/p>\n
std::optional<SomeKindOfDataBuffer>\r\nWidget::SaveToBuffer(\r\n    winrt::Iterator<winrt::IKeyValuePair<winrt::hstring,\r\n                                         winrt::IInspectable>> it<\/span>)\r\n{\r\n    try {\r\n        SomeKindOfDataBuffer buffer;\r\n        \/\/ auto it = m_propertySet.First();<\/span><\/span>\r\n        if (it.HasCurrent()) {\r\n            do {\r\n                auto current = it.Current();\r\n                buffer.AddKeyAndValue(current.Key(), current.Value());\r\n            } while (it.MoveNext());\r\n        }\r\n\r\n        \/\/ auto guard = m_lock.lock();<\/span>\r\n\r\n        \/\/ verify that the collection is still unchanged before returning<\/span>\r\n        \/\/ std::ignore = it.HasCurrent();<\/span><\/span>\r\n\r\n        return buffer<\/span>;\r\n    } catch (winrt::hresult_changed_state const&) {\r\n        \/\/ Abandon the operation.\r\n        return std::nullopt;<\/span>\r\n    }\r\n}\r\n\r\nvoid Widget::OnPropertySetChanged()\r\n{\r\n    auto it = m_propertySet.First();<\/span>\r\n    auto buffer = SaveToBuffer(it);\r\n    if (buffer) {\r\n        auto guard = m_lock.lock();\r\n        try {\r\n            \/\/ final check under the lock\r\n            std::ignore = it.HasCurrent();\r\n            SaveToAutoSaveLocation(*buffer);\r\n        } catch (winrt::hresult_changed_state const&) {\r\n            \/\/ Abandon the operation.\r\n        }\r\n    }\r\n}\r\n\r\nvoid Widget::Save()\r\n{\r\n    auto it = m_propertySet.First();<\/span>\r\n    auto buffer = SaveToBuffer(it);\r\n\r\n    \/\/ Verify that the collection wasn't changed\r\n    \/\/ while we were trying to build the buffer.\r\n    std::ignore = it.HasCurrent();\r\n\r\n    SaveBuffer(*buffer);\r\n}\r\n<\/pre>\n
But the pattern I am currently a fan of is to make the caller do the exception handling, too:<\/p>\n
SomeKindOfDataBuffer Widget::SaveToBuffer()\r\n{\r\n    SomeKindOfDataBuffer buffer;\r\n    for (auto [name, value] : m_propertySet)\r\n        buffer.AddKeyAndValue(name, value);\r\n    }\r\n    return buffer;\r\n}\r\n\r\nvoid Widget::OnPropertySetChanged() try\r\n{\r\n    auto it = m_propertySet.First();\r\n    auto buffer = SaveToBuffer();\r\n\r\n    auto guard = m_lock.lock();\r\n    \/\/ final check under the lock\r\n    std::ignore = it.HasCurrent();\r\n\r\n    SaveToAutoSaveLocation(buffer);\r\n} catch (winrt::hresult_changed_state const&) {\r\n    \/\/ Abandon the operation.\r\n}\r\n\r\nvoid Widget::Save()\r\n{\r\n    auto buffer = SaveToBuffer();\r\n    SaveBuffer(buffer);\r\n}\r\n<\/pre>\n","protected":false},"excerpt":{"rendered":"
Processing the result differently.<\/p>\n","protected":false},"author":1069,"featured_media":111744,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[1],"tags":[25],"class_list":["post-106866","post","type-post","status-publish","format-standard","has-post-thumbnail","hentry","category-oldnewthing","tag-code"],"acf":[],"blog_post_summary":"
Processing the result differently.<\/p>\n","_links":{"self":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/106866","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/users\/1069"}],"replies":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/comments?post=106866"}],"version-history":[{"count":0,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/106866\/revisions"}],"wp:featuredmedia":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media\/111744"}],"wp:attachment":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media?parent=106866"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/categories?post=106866"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/tags?post=106866"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}