event_revoker ignores errors while manual unregistration does not

[sylveon]

As can be seen here, the hresult return is ignored:

cppwinrt/strings/base_events.h

Lines 57 to 63 in 7e730e7

	void revoke() noexcept
	{
	if (I object = std::exchange(m_object, {}).get())
	{
	((*reinterpret_cast<impl::abi_t<I>**>(&object))->*(m_method))(m_token);
	}
	}

However, in the code generated for manual unregistration, the return value is asserted to be S_OK:

cppwinrt/cppwinrt/code_writers.h

Lines 1121 to 1128 in 4366357

	if (is_noexcept(method))
	{
	format = R"( template <typename D%> WINRT_IMPL_AUTO(%) consume_%<D%>::%(%) const noexcept
	{%
	WINRT_VERIFY_(0, WINRT_IMPL_SHIM(%)->%(%));%
	}
	)";
	}

This means that the behavior is different: I was migrating code to use manual unregistration because it was already holding a strong reference so storing a single event_token takes much less storage. While event_revoker worked fine, manual unregistration triggers the assert if the RPC server has died while trying to unregister the callback (because then the method to unregister the callback returns RPC_S_SERVER_UNAVAILABLE).

[kennykerr]

Yep, the event revoker came much later and by then I realized there was little point in propagating errors resulting from unregistering an event. Ideally we could make the manual unregistration behave the same way. I can't imagine anyone depending on this, but you never know.

[sylveon]

People have probably inadvertently started depending on event_revoker ignoring errors (as I did, until I tried manual unregistration), and currently manual unregistration errors are a fatal failure in debug mode and ignored in release mode.

So I don't think anybody depends on asserts firing (because they terminate, and sudden termination is something nobody really depends on), and the "correct" behavior already exists in release mode. Ignoring errors in manual unregistration will bring consistency and should not result in any breakages.

[kennykerr]

Agreed.