{"id":107103,"date":"2022-09-02T07:00:00","date_gmt":"2022-09-02T14:00:00","guid":{"rendered":"https:\/\/devblogs.microsoft.com\/oldnewthing\/?p=107103"},"modified":"2022-08-31T07:13:16","modified_gmt":"2022-08-31T14:13:16","slug":"20220902-00","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/oldnewthing\/20220902-00\/?p=107103","title":{"rendered":"The case of the recursively-acquired non-recursive lock, and how to avoid the unintentional reentrancy"},"content":{"rendered":"

A customer encountered a deadlock due to unexpected reentrancy, and they were looking for guidance in fixing it.<\/p>\n

Here’s the code in question:<\/p>\n

struct WidgetTracker : IWidgetChangeNotificationSink\r\n{\r\n    \/* other stuff not relevant here *\/\r\n\r\n    \/\/\/ IWidgetChangeNotificationSink\r\n    STDMETHODIMP OnCurrentWidgetChanged();\r\n\r\nprivate:\r\n    WRL::ComPtr<IWidget> m_currentWidget;\r\n    std::mutex m_mutex;\r\n};\r\n\r\nHRESULT WidgetTracker::OnCurrentWidgetChanged()\r\n{\r\n    auto guard = std::lock_guard(m_mutex);\r\n    RETURN_IF_FAILED(GetCurrentWidget(&m_currentWidget));\r\n    return S_OK;\r\n}\r\n<\/pre>\n
The idea here is that the WidgetTracker<\/code> listens for notifications that the current widget has changed, and when it receives that notification, it updates its local cache to hold the new current widget.<\/p>\n
The hang occurred with this stack:<\/p>\n
ntdll!ZwWaitForAlertByThreadId\r\nntdll!RtlAcquireSRWLockExclusive\r\ncontoso!WidgetTracker::OnCurrentWidgetChanged\r\nrpcrt4!Invoke\r\nrpcrt4!Ndr64StubWorker\r\nrpcrt4!NdrStubCall3\r\ncombase!CStdStubBuffer_Invoke\r\ncombase!InvokeStubWithExceptionPolicyAndTracing::__l6::<lambda_...>::operator()\r\ncombase!ObjectMethodExceptionHandlingAction<<lambda_...> >\r\ncombase!InvokeStubWithExceptionPolicyAndTracing\r\ncombase!DefaultStubInvoke\r\ncombase!SyncServerCall::StubInvoke\r\ncombase!StubInvoke\r\ncombase!ServerCall::ContextInvoke\r\ncombase!DefaultInvokeInApartment\r\ncombase!ReentrantSTAInvokeInApartment\r\ncombase!ComInvokeWithLockAndIPID\r\ncombase!ThreadDispatch\r\ncombase!ThreadWndProc\r\nuser32!UserCallWinProcCheckWow\r\nuser32!DispatchMessageWorker\r\ncombase!CCliModalLoop::MyDispatchMessage\r\ncombase!CCliModalLoop::PeekRPCAndDDEMessage\r\ncombase!CCliModalLoop::BlockFn\r\ncombase!ModalLoop\r\ncombase!ThreadSendReceive\r\ncombase!CSyncClientCall::SwitchAptAndDispatchCall\r\ncombase!CSyncClientCall::SendReceive2\r\ncombase!SyncClientCallRetryContext::SendReceiveWithRetry\r\ncombase!CSyncClientCall::SendReceiveInRetryContext\r\ncombase!ClassicSTAThreadSendReceive\r\ncombase!CSyncClientCall::SendReceive\r\ncombase!CClientChannel::SendReceive\r\ncombase!NdrExtpProxySendReceive\r\nrpcrt4!Ndr64pSendReceive\r\nrpcrt4!NdrpClientCall3\r\ncombase!ObjectStublessClient\r\ncombase!ObjectStubless\r\nlitware!Widget::~Widget\r\nlitware!Widget::`scalar deleting destructor'\r\nlitware!Widget::Release\r\ncontoso!Microsoft::WRL::ComPtr<IWidget>::InternalRelease\r\ncontoso!Microsoft::WRL::ComPtr<IWidget>::ReleaseAndGetAddressOf\r\ncontoso!Microsoft::WRL::Details::ComPtrRef<...>::operator struct IWidget **\r\ncontoso!WidgetTracker::OnCurrentWidgetChanged\r\nrpcrt4!Invoke\r\nrpcrt4!Ndr64StubWorker\r\nrpcrt4!NdrStubCall3\r\ncombase!CStdStubBuffer_Invoke\r\ncombase!InvokeStubWithExceptionPolicyAndTracing::__l6::<lambda_...>::operator()\r\ncombase!ObjectMethodExceptionHandlingAction<<lambda_...> >\r\ncombase!InvokeStubWithExceptionPolicyAndTracing\r\n...\r\n<\/pre>\n
Reading from the bottom up, what happened is that the current widget changed, and the Widget\u00adTracker<\/code> received the change notification. The Widget\u00adTracker<\/code> locks the mutex, and then wants to get the new current Widget<\/code>, but first it releases the old Widget<\/code>.<\/p>\n
It is that release of the old widget that causes trouble, because it makes a cross-process call, and while waiting for the cross-process call to complete, the current widget changes again, and the On\u00adCurrent\u00adWidget\u00adChanged<\/code> method gets called again. (It is evident that this code is running on a single-threaded apartment. If it were running in a multi-threaded apartment, the second call would have arrived on a different thread.)<\/p>\n
The problem is that we are releasing our reference to the old widget while holding a lock, and that creates the opportunity for mayhem, since we don’t control what the widget will do when it is released. And if this is the final release of the widget, it will probably do a lot of work.<\/p>\n
This is another case of  the hidden callout: The destructor<\/a>.<\/p>\n
And the solution is the same: Destruct the reference to the old widget outside the lock.<\/p>\n
HRESULT WidgetTracker::OnCurrentWidgetChanged()\r\n{\r\n    WRL::ComPtr<IWidget> widget;\r\n    auto guard = std::lock_guard(m_mutex);\r\n    RETURN_IF_FAILED(GetCurrentWidget(&widget));\r\n    m_currentWidget.Swap(widget);\r\n    return S_OK;\r\n}\r\n<\/pre>\n
We declare a ComPtr<IWidget><\/code> before taking the lock, so that it destructs after the lock is released. (Remember that in C++, local variables are destructed in reverse order of construction.) After we get the current widget into the local widget<\/code>, we swap it with the old one, and then return.<\/p>\n
The lock guard destructs first, which exits the lock. and then the ComPtr<IWidget><\/code> destructs, which releases the old widget. This release occurs outside the lock, so any re-entrancy is not going to create a deadlock.<\/p>\n","protected":false},"excerpt":{"rendered":"
Watch what you do when you hold a lock.<\/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-107103","post","type-post","status-publish","format-standard","has-post-thumbnail","hentry","category-oldnewthing","tag-code"],"acf":[],"blog_post_summary":"
Watch what you do when you hold a lock.<\/p>\n","_links":{"self":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/107103","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=107103"}],"version-history":[{"count":0,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/107103\/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=107103"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/categories?post=107103"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/tags?post=107103"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}