From 84a3654ae81ce1b08c25afca1befc9b38b4aa986 Mon Sep 17 00:00:00 2001 From: Scott Jones Date: Thu, 17 Jun 2021 13:31:36 -0700 Subject: [PATCH] [DO NOT MERGE] conditional revert of EventSource caching (#876) * Revert "Added caching of event registration state to support unsubscribes across garbage collections (#861)" This reverts commit fa038af4ce732e34163d839d2c5e1bc0b0ae3ac8. * fix merge bug and make weak ref interfaces public * Add back null check to avoid future issues if older runtime is used. Co-authored-by: Manodasan Wignarajah --- src/Tests/TestComponentCSharp/Singleton.cpp | 43 ---- src/Tests/TestComponentCSharp/Singleton.h | 19 -- .../TestComponentCSharp.idl | 13 +- .../TestComponentCSharp.vcxproj | 2 - .../TestComponentCSharp.vcxproj.filters | 2 - .../UnitTest/TestComponentCSharp_Tests.cs | 26 +- src/WinRT.Runtime/ComWrappersSupport.cs | 2 +- src/cswinrt/code_writers.h | 117 ++++----- src/cswinrt/cswinrt.vcxproj | 3 - src/cswinrt/strings/WinRT.cs | 222 +++++------------- 10 files changed, 110 insertions(+), 339 deletions(-) delete mode 100644 src/Tests/TestComponentCSharp/Singleton.cpp delete mode 100644 src/Tests/TestComponentCSharp/Singleton.h diff --git a/src/Tests/TestComponentCSharp/Singleton.cpp b/src/Tests/TestComponentCSharp/Singleton.cpp deleted file mode 100644 index 541403f6c..000000000 --- a/src/Tests/TestComponentCSharp/Singleton.cpp +++ /dev/null @@ -1,43 +0,0 @@ -#include "pch.h" -#include "Singleton.h" -#include "Singleton.g.cpp" - -using namespace winrt; -using namespace Windows::Foundation; - -namespace winrt::TestComponentCSharp::implementation -{ - TestComponentCSharp::ISingleton Singleton::Instance() - { - struct singleton : winrt::implements - { - int _int{}; - winrt::event> _intChanged {}; - - int32_t IntProperty() - { - return _int; - } - void IntProperty(int32_t value) - { - _int = value; - _intChanged(nullptr, _int); - } - winrt::event_token IntPropertyChanged(EventHandler const& handler) - { - return _intChanged.add(handler); - } - void IntPropertyChanged(winrt::event_token const& token) noexcept - { - _intChanged.remove(token); - } - }; - static TestComponentCSharp::ISingleton _singleton = winrt::make(); - return _singleton; - } - - void Singleton::Instance(TestComponentCSharp::ISingleton const& value) - { - throw hresult_not_implemented(); - } -} diff --git a/src/Tests/TestComponentCSharp/Singleton.h b/src/Tests/TestComponentCSharp/Singleton.h deleted file mode 100644 index aa02e6e64..000000000 --- a/src/Tests/TestComponentCSharp/Singleton.h +++ /dev/null @@ -1,19 +0,0 @@ -#pragma once -#include "Singleton.g.h" - -namespace winrt::TestComponentCSharp::implementation -{ - struct Singleton - { - Singleton() = default; - - static TestComponentCSharp::ISingleton Instance(); - static void Instance(TestComponentCSharp::ISingleton const& value); - }; -} -namespace winrt::TestComponentCSharp::factory_implementation -{ - struct Singleton : SingletonT - { - }; -} diff --git a/src/Tests/TestComponentCSharp/TestComponentCSharp.idl b/src/Tests/TestComponentCSharp/TestComponentCSharp.idl index 8bbee2db4..e9d3f0de3 100644 --- a/src/Tests/TestComponentCSharp/TestComponentCSharp.idl +++ b/src/Tests/TestComponentCSharp/TestComponentCSharp.idl @@ -130,17 +130,6 @@ namespace TestComponentCSharp static Int32 NumObjects{ get; }; } - interface ISingleton - { - Int32 IntProperty; - event Windows.Foundation.EventHandler IntPropertyChanged; - } - - static runtimeclass Singleton - { - static ISingleton Instance; - } - [default_interface, gc_pressure(Windows.Foundation.Metadata.GCPressureAmount.High)] runtimeclass Class : Windows.Foundation.IStringable @@ -464,7 +453,7 @@ namespace TestComponentCSharp static event Windows.Foundation.EventHandler WarningEvent; } } - + [contract(Windows.Foundation.UniversalApiContract, 8)] interface IWarning1 { diff --git a/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj b/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj index 56465fd5a..01d56f880 100644 --- a/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj +++ b/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj @@ -71,7 +71,6 @@ - @@ -87,7 +86,6 @@ - diff --git a/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj.filters b/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj.filters index 472076c79..91f509b7c 100644 --- a/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj.filters +++ b/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj.filters @@ -15,7 +15,6 @@ - @@ -25,7 +24,6 @@ - diff --git a/src/Tests/UnitTest/TestComponentCSharp_Tests.cs b/src/Tests/UnitTest/TestComponentCSharp_Tests.cs index fef164485..537fa19e5 100644 --- a/src/Tests/UnitTest/TestComponentCSharp_Tests.cs +++ b/src/Tests/UnitTest/TestComponentCSharp_Tests.cs @@ -2438,14 +2438,15 @@ public void TestCovariance() Assert.True(TestObject.IterableOfObjectIterablesProperty.SequenceEqual(listOfListOfUris)); } - // Ensure that event subscription state is properly cached to enable later unsubscribes [Fact] - public void TestEventSourceCaching() + public void TestStaticEventWithGC() { bool eventCalled = false; - void Class_StaticIntPropertyChanged(object sender, int e) => eventCalled = (e == 3); + void Class_StaticIntPropertyChanged(object sender, int e) + { + eventCalled = (e == 3); + } - // Test static codegen-based EventSource caching Class.StaticIntPropertyChanged += Class_StaticIntPropertyChanged; GC.Collect(2, GCCollectionMode.Forced, true); GC.WaitForPendingFinalizers(); @@ -2457,23 +2458,6 @@ public void TestEventSourceCaching() GC.WaitForPendingFinalizers(); Class.StaticIntProperty = 3; Assert.True(eventCalled); - - // Test dynamic WeakRef-based EventSource caching - eventCalled = false; - static void Subscribe(EventHandler handler) => Singleton.Instance.IntPropertyChanged += handler; - static void Unsubscribe(EventHandler handler) => Singleton.Instance.IntPropertyChanged -= handler; - static void Assign(int value) => Singleton.Instance.IntProperty = value; - Subscribe(Class_StaticIntPropertyChanged); - GC.Collect(2, GCCollectionMode.Forced, true); - GC.WaitForPendingFinalizers(); - Unsubscribe(Class_StaticIntPropertyChanged); - Assign(3); - Assert.False(eventCalled); - Subscribe(Class_StaticIntPropertyChanged); - GC.Collect(2, GCCollectionMode.Forced, true); - GC.WaitForPendingFinalizers(); - Assign(3); - Assert.True(eventCalled); } #if NET5_0 diff --git a/src/WinRT.Runtime/ComWrappersSupport.cs b/src/WinRT.Runtime/ComWrappersSupport.cs index 51ab7715a..c8e705abd 100644 --- a/src/WinRT.Runtime/ComWrappersSupport.cs +++ b/src/WinRT.Runtime/ComWrappersSupport.cs @@ -60,7 +60,7 @@ public static IObjectReference GetObjectReferenceForInterface(IntPtr externalCom { if (externalComObject == IntPtr.Zero) { - return null; + return null; } using var unknownRef = ObjectReference.FromAbi(externalComObject); diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index e684483ce..dab3687f1 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -2517,7 +2517,7 @@ db_path.stem().string()); void write_event_source_generic_args(writer& w, cswinrt::type_semantics eventTypeSemantics); - void write_event_source_ctor(writer& w, Event const& evt, int index) + void write_event_source_ctor(writer& w, Event const& evt) { if (for_typedef(w, get_type_semantics(evt.EventType()), [&](TypeDef const& eventType) { @@ -2526,12 +2526,10 @@ db_path.stem().string()); auto [add, remove] = get_event_methods(evt); w.write(R"( new EventSource__EventHandler%(_obj, %, -%, %))", bind(eventType), get_invoke_info(w, add).first, -get_invoke_info(w, remove).first, -index); +get_invoke_info(w, remove).first); return true; } return false; @@ -2542,15 +2540,13 @@ index); auto [add, remove] = get_event_methods(evt); w.write(R"( -new %%(_obj, -%, -%, -%))", + new %%(_obj, + %, + %))", bind(get_type_semantics(evt.EventType())), bind(get_type_semantics(evt.EventType())), get_invoke_info(w, add).first, - get_invoke_info(w, remove).first, - index); + get_invoke_info(w, remove).first); } void write_event_sources(writer& w, TypeDef const& type) @@ -3458,37 +3454,26 @@ global::System.Collections.Concurrent.ConcurrentDictionary +{ +% +return %; +}))", + evt.Name(), + bind(init_call_variables), + bind(evt)); w.write(R"( -%%event % %% +%event % %% { add => %.Subscribe(value); remove => %.Unsubscribe(value); } )", - bind([&](writer& w) - { - if(settings.netstandard_compat) - return; - w.write(R"(private EventSource<%> Get_%() -{ -return _%.GetValue((IWinRTObject)this, (key) => -{ -% -return %; -}); -} -)", - bind(get_type_semantics(evt.EventType()), typedef_name_type::Projected, false), - evt.Name(), - evt.Name(), - bind(init_call_variables), - bind(evt, index)); - }), settings.netstandard_compat ? "public " : "", bind(get_type_semantics(evt.EventType()), typedef_name_type::Projected, false), bind([&](writer& w) @@ -3501,7 +3486,6 @@ return %; evt.Name(), event_source, event_source); - index++; } } @@ -4967,14 +4951,11 @@ public static Guid PIID = Vftbl.PIID; type_name, type.TypeName(), type.TypeName(), - [&](writer& w) + bind_each([&](writer& w, Event const& evt) { - int index = 0; - for (auto&& evt : type.EventList()) - { - w.write("_% = %;\n", evt.Name(), bind(evt, index++)); - } + w.write("_% = %;\n", evt.Name(), bind(evt)); }, + type.EventList()), [&](writer& w) { for (auto required_interface : required_interfaces) { @@ -6436,7 +6417,7 @@ bind(type, typedef_name_type::CCW, true) { return; } - for_typedef(w, eventTypeSemantics, [&](TypeDef const&) + for_typedef(w, eventTypeSemantics, [&](TypeDef const& eventType) { std::vector genericArgs; auto arg_count = std::get(eventTypeSemantics).generic_args.size(); @@ -6470,36 +6451,36 @@ bind(type, typedef_name_type::CCW, true) auto eventTypeCode = w.write_temp("%", bind(eventType, typedef_name_type::Projected, false)); auto invokeMethodSig = get_event_invoke_method(eventType); w.write(R"( - internal unsafe class %% : EventSource<%> - { - private % handler; +internal unsafe class %% : EventSource<%> +{ +private % handler; - internal %(IObjectReference obj, - delegate* unmanaged[Stdcall] addHandler, - delegate* unmanaged[Stdcall] removeHandler, int index) : base(obj, addHandler, removeHandler, index) - { - } +internal %(IObjectReference obj, +delegate* unmanaged[Stdcall] addHandler, +delegate* unmanaged[Stdcall] removeHandler) : base(obj, addHandler, removeHandler) +{ +} - override protected System.Delegate EventInvoke - { - get - { - if (handler == null) - { - handler = (%) => - { - var localDel = _state.del; - if (localDel == null) - { - return %; - } - %localDel.Invoke(%); - }; - } - return handler; - } - } - } +override protected System.Delegate EventInvoke +{ +get +{ +if (handler == null) +{ +handler = (%) => +{ +var localDel = _event; +if (localDel == null) +{ +return %; +} +%localDel.Invoke(%); +}; +} +return handler; +} +} +} )", bind(eventTypeSemantics), bind(eventTypeSemantics), diff --git a/src/cswinrt/cswinrt.vcxproj b/src/cswinrt/cswinrt.vcxproj index 674c12582..9856448e4 100644 --- a/src/cswinrt/cswinrt.vcxproj +++ b/src/cswinrt/cswinrt.vcxproj @@ -46,9 +46,6 @@ Console kernel32.lib;user32.lib;%(AdditionalDependencies);windowsapp.lib;advapi32.lib;shlwapi.lib - - true - diff --git a/src/cswinrt/strings/WinRT.cs b/src/cswinrt/strings/WinRT.cs index a129626a7..22a72d42a 100644 --- a/src/cswinrt/strings/WinRT.cs +++ b/src/cswinrt/strings/WinRT.cs @@ -18,7 +18,7 @@ namespace WinRT { - using System.Diagnostics; + using System.Diagnostics; using WinRT.Interop; internal static class DelegateExtensions @@ -142,8 +142,8 @@ public static DllModule Load(string fileName) #if !NETSTANDARD2_0 && !NETCOREAPP2_0 if (_moduleHandle == IntPtr.Zero) { - try - { + try + { // Allow runtime to find module in RID-specific relative subfolder _moduleHandle = NativeLibrary.Load(fileName, Assembly.GetExecutingAssembly(), null); } @@ -244,8 +244,8 @@ public static unsafe (ObjectReference obj, int hr) GetA { m.Dispose(); } - } - + } + ~WinrtModule() { Marshal.ThrowExceptionForHR(Platform.CoDecrementMTAUsage(_mtaCookie)); @@ -338,18 +338,11 @@ internal unsafe class EventSource where TDelegate : class, MulticastDelegate { readonly IObjectReference _obj; - readonly int _index; readonly delegate* unmanaged[Stdcall] _addHandler; readonly delegate* unmanaged[Stdcall] _removeHandler; - // Registration state, cached separately to survive EventSource garbage collection - protected class State - { - public EventRegistrationToken token; - public TDelegate del; - public System.Delegate eventInvoke; - } - protected State _state; + private EventRegistrationToken _token; + protected TDelegate _event; protected virtual IObjectReference CreateMarshaler(TDelegate del) { @@ -370,16 +363,16 @@ public void Subscribe(TDelegate del) { lock (this) { - bool registerHandler = _state.del is null; + bool registerHandler = _event is null; - _state.del = (TDelegate)global::System.Delegate.Combine(_state.del, del); + _event = (TDelegate)global::System.Delegate.Combine(_event, del); if (registerHandler) { var marshaler = CreateMarshaler((TDelegate)EventInvoke); try { var nativeDelegate = GetAbi(marshaler); - ExceptionHelpers.ThrowExceptionForHR(_addHandler(_obj.ThisPtr, nativeDelegate, out _state.token)); + ExceptionHelpers.ThrowExceptionForHR(_addHandler(_obj.ThisPtr, nativeDelegate, out _token)); } finally { @@ -395,22 +388,23 @@ public void Unsubscribe(TDelegate del) { lock (this) { - var oldEvent = _state.del; - _state.del = (TDelegate)global::System.Delegate.Remove(_state.del, del); - if (oldEvent is object && _state.del is null) + var oldEvent = _event; + _event = (TDelegate)global::System.Delegate.Remove(_event, del); + if (oldEvent is object && _event is null) { _UnsubscribeFromNative(); } } } + private System.Delegate _eventInvoke; protected virtual System.Delegate EventInvoke { get { - if (_state.eventInvoke is object) + if (_eventInvoke is object) { - return _state.eventInvoke; + return _eventInvoke; } MethodInfo invoke = typeof(TDelegate).GetMethod("Invoke"); @@ -423,172 +417,64 @@ protected virtual System.Delegate EventInvoke ParameterExpression delegateLocal = Expression.Parameter(typeof(TDelegate), "event"); - _state.eventInvoke = Expression.Lambda(typeof(TDelegate), + _eventInvoke = Expression.Lambda(typeof(TDelegate), Expression.Block( invoke.ReturnType, new[] { delegateLocal }, - Expression.Assign(delegateLocal, Expression.Field(Expression.Constant(_state), typeof(EventSource.State).GetField(nameof(_state.del), BindingFlags.Instance | BindingFlags.Public))), + Expression.Assign(delegateLocal, Expression.Field(Expression.Constant(this), typeof(EventSource).GetField(nameof(_event), BindingFlags.Instance | BindingFlags.NonPublic))), Expression.Condition( Expression.ReferenceNotEqual(delegateLocal, Expression.Constant(null, typeof(TDelegate))), Expression.Call(delegateLocal, invoke, parameters), Expression.Default(invoke.ReturnType))), parameters).Compile(); - return _state.eventInvoke; - } - } - - private class Cache - { - Cache(IWeakReference target, EventSource source, int index) - { - this.target = target; - SetState(source, index); - } - - private IWeakReference target; - private readonly ConcurrentDictionary.State> states = new ConcurrentDictionary.State>(); - - private static readonly ReaderWriterLockSlim cachesLock = new ReaderWriterLockSlim(); - private static readonly ConcurrentDictionary caches = new ConcurrentDictionary(); - - private Cache Update(IWeakReference target, EventSource source, int index) - { - // If target no longer exists, destroy cache - lock (this) - { - using var resolved = this.target.Resolve(typeof(IUnknownVftbl).GUID); - if (resolved == null) - { - this.target = target; - states.Clear(); - } - } - SetState(source, index); - return this; - } - - private void SetState(EventSource source, int index) - { - // If cache exists, use it, else create new - if (states.ContainsKey(index)) - { - source._state = states[index]; - } - else - { - source._state = new EventSource.State(); - states[index] = source._state; - } - } - - public static void Create(IObjectReference obj, EventSource source, int index) - { - // If event source implements weak reference support, track event registrations so that - // unsubscribes will work across garbage collections. Note that most static/factory classes - // do not implement IWeakReferenceSource, so static codegen caching approach is also used. - IWeakReference target = null; - try - { -#if NETSTANDARD2_0 - var weakRefSource = (IWeakReferenceSource)typeof(IWeakReferenceSource).GetHelperType().GetConstructor(new[] { typeof(IObjectReference) }).Invoke(new object[] { obj }); -#else - var weakRefSource = (IWeakReferenceSource)(object)new WinRT.IInspectable(obj); -#endif - target = weakRefSource.GetWeakReference(); - } - catch (Exception) - { - source._state = new EventSource.State(); - return; - } - - cachesLock.EnterReadLock(); - try - { - caches.AddOrUpdate(obj.ThisPtr, - (IntPtr ThisPtr) => new Cache(target, source, index), - (IntPtr ThisPtr, Cache cache) => cache.Update(target, source, index)); - } - finally - { - cachesLock.ExitReadLock(); - } - } - - public static void Remove(IntPtr thisPtr, int index) - { - if (caches.TryGetValue(thisPtr, out var cache)) - { - cache.states.TryRemove(index, out var _); - // using double-checked lock idiom - if (cache.states.IsEmpty) - { - cachesLock.EnterWriteLock(); - try - { - if (cache.states.IsEmpty) - { - caches.TryRemove(thisPtr, out var _); - } - } - finally - { - cachesLock.ExitWriteLock(); - } - } - } + return _eventInvoke; } } internal EventSource(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, - delegate* unmanaged[Stdcall] removeHandler, - int index = 0) + delegate* unmanaged[Stdcall] removeHandler) { - Cache.Create(obj, this, index); _obj = obj; - _index = index; _addHandler = addHandler; _removeHandler = removeHandler; } void _UnsubscribeFromNative() { - Cache.Remove(_obj.ThisPtr, _index); - ExceptionHelpers.ThrowExceptionForHR(_removeHandler(_obj.ThisPtr, _state.token)); - _state.token.Value = 0; + ExceptionHelpers.ThrowExceptionForHR(_removeHandler(_obj.ThisPtr, _token)); + _token.Value = 0; } } - internal unsafe class EventSource__EventHandler : EventSource> - { - private System.EventHandler handler; - - internal EventSource__EventHandler(IObjectReference obj, - delegate* unmanaged[Stdcall] addHandler, - delegate* unmanaged[Stdcall] removeHandler, - int index) : base(obj, addHandler, removeHandler, index) - { - } - - override protected System.Delegate EventInvoke - { - // This is synchronized from the base class - get - { - if (handler == null) - { - handler = (System.Object obj, T e) => + internal unsafe class EventSource__EventHandler : EventSource> + { + private System.EventHandler handler; + + internal EventSource__EventHandler(IObjectReference obj, + delegate* unmanaged[Stdcall] addHandler, + delegate* unmanaged[Stdcall] removeHandler) : base(obj, addHandler, removeHandler) + { + } + + override protected System.Delegate EventInvoke + { + // This is synchronized from the base class + get + { + if (handler == null) + { + handler = (System.Object obj, T e) => { - var localDel = _state.del; - if (localDel != null) - localDel.Invoke(obj, e); - }; - } - return handler; - } - } + var localDel = _event; + if (localDel != null) + localDel.Invoke(obj, e); + }; + } + return handler; + } + } } #pragma warning restore CA2002 @@ -727,13 +613,13 @@ namespace WinRT { using System.Runtime.CompilerServices; internal static class ProjectionInitializer - { + { #pragma warning disable 0436 - [ModuleInitializer] + [ModuleInitializer] #pragma warning restore 0436 - internal static void InitalizeProjection() - { - ComWrappersSupport.RegisterProjectionAssembly(typeof(ProjectionInitializer).Assembly); - } + internal static void InitalizeProjection() + { + ComWrappersSupport.RegisterProjectionAssembly(typeof(ProjectionInitializer).Assembly); + } } }