Skip to content

Commit aa89113

Browse files
retailcoderretailcoder
authored
Merge pull request #3769 from MDoerner/EnhanceTheComSafe
Introduces weak-reference COM safe; all SafeComWrappers are now garbage-collected.
2 parents 961a30a + fb21d14 commit aa89113

File tree

7 files changed

+150
-55
lines changed

7 files changed

+150
-55
lines changed

‎RetailCoder.VBE/Extension.cs

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ public class _Extension : IDTExtensibility2
4242

4343
private IWindsorContainer _container;
4444
private App _app;
45-
private IComSafe _comSafe;
4645
private readonly Logger _logger = LogManager.GetCurrentClassLogger();
4746

4847
public void OnAddInsUpdate(ref Array custom) { }
@@ -52,9 +51,6 @@ public void OnConnection(object Application, ext_ConnectMode ConnectMode, object
5251
{
5352
try
5453
{
55-
ComSafeManager.ResetComSafe();
56-
_comSafe = ComSafeManager.GetCurrentComSafe();
57-
5854
if (Application is Microsoft.Vbe.Interop.VBE vbe1)
5955
{
6056
_ide = new VBEditor.SafeComWrappers.VBA.VBE(vbe1);
@@ -270,18 +266,6 @@ private void ShutdownAddIn()
270266
_container.Dispose();
271267
_container = null;
272268
}
273-
274-
if (_comSafe != null)
275-
{
276-
_logger.Log(LogLevel.Trace, "Disposing COM safe...");
277-
_comSafe.Dispose();
278-
_comSafe = null;
279-
_addin = null;
280-
_ide = null;
281-
}
282-
283-
_isInitialized = false;
284-
_logger.Log(LogLevel.Info, "No exceptions were thrown.");
285269
}
286270
catch (Exception e)
287271
{
@@ -291,11 +275,30 @@ private void ShutdownAddIn()
291275
}
292276
finally
293277
{
294-
_logger.Log(LogLevel.Trace, "Unregistering AppDomain handlers....");
295-
currentDomain.AssemblyResolve -= LoadFromSameFolder;
296-
currentDomain.UnhandledException -= HandlAppDomainException;
297-
_logger.Log(LogLevel.Trace, "Done. Main Shutdown completed. Toolwindows follow. Quack!");
298-
_isInitialized = false;
278+
try
279+
{
280+
_logger.Log(LogLevel.Trace, "Disposing COM safe...");
281+
ComSafeManager.DisposeAndResetComSafe();
282+
_addin = null;
283+
_ide = null;
284+
285+
_isInitialized = false;
286+
_logger.Log(LogLevel.Info, "No exceptions were thrown.");
287+
}
288+
catch (Exception e)
289+
{
290+
_logger.Error(e);
291+
_logger.Log(LogLevel.Warn, "Exception disposing the ComSafe has been swallowed.");
292+
//throw; // <<~ uncomment to crash the process
293+
}
294+
finally
295+
{
296+
_logger.Log(LogLevel.Trace, "Unregistering AppDomain handlers....");
297+
currentDomain.AssemblyResolve -= LoadFromSameFolder;
298+
currentDomain.UnhandledException -= HandlAppDomainException;
299+
_logger.Log(LogLevel.Trace, "Done. Main Shutdown completed. Toolwindows follow. Quack!");
300+
_isInitialized = false;
301+
}
299302
}
300303
}
301304
}

‎Rubberduck.VBEEditor/ComManagement/ComSafeManager.cs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
using System;
2+
3+
namespace Rubberduck.VBEditor.ComManagement
4+
{
5+
public static class ComSafeManager
6+
{
7+
private static Lazy<IComSafe> _comSafe = new Lazy<IComSafe>(NewComSafe);
8+
9+
public static IComSafe GetCurrentComSafe()
10+
{
11+
return _comSafe.Value;
12+
}
13+
14+
public static void DisposeAndResetComSafe()
15+
{
16+
var oldComSafe = _comSafe.Value;
17+
_comSafe = new Lazy<IComSafe>(NewComSafe);
18+
oldComSafe.Dispose();
19+
}
20+
21+
private static IComSafe NewComSafe()
22+
{
23+
return new WeakComSafe();
24+
}
25+
}
26+
}

‎Rubberduck.VBEEditor/ComManagement/ComSafe.cs renamed to ‎Rubberduck.VBEEditor/ComManagement/StrongComSafe.cs

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
namespace Rubberduck.VBEditor.ComManagement
66
{
7-
public class ComSafe: IComSafe
7+
public class StrongComSafe: IComSafe
88
{
99
//We override the equality comparison and hash code because subclasses of SafeComWrapper<T> override the corresponding methods.
1010
//We need to distinguish between the individual wrapper instances no matter whether they are semantically equal.
@@ -43,19 +43,4 @@ public void Dispose()
4343
_comWrapperCache.Clear();
4444
}
4545
}
46-
47-
public static class ComSafeManager
48-
{
49-
private static Lazy<ComSafe> _comSafe = new Lazy<ComSafe>();
50-
51-
public static IComSafe GetCurrentComSafe()
52-
{
53-
return _comSafe.Value;
54-
}
55-
56-
public static void ResetComSafe()
57-
{
58-
_comSafe = new Lazy<ComSafe>();
59-
}
60-
}
6146
}

‎Rubberduck.VBEEditor/ComManagement/WeakComSafe.cs

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
using System;
2+
using System.Collections.Concurrent;
3+
using System.Runtime.CompilerServices;
4+
using Rubberduck.VBEditor.SafeComWrappers.Abstract;
5+
6+
namespace Rubberduck.VBEditor.ComManagement
7+
{
8+
public class WeakComSafe : IComSafe
9+
{
10+
//We use weak references to allow the GC to reclaim RCWs earlier if possible.
11+
private readonly ConcurrentDictionary<int, WeakReference<ISafeComWrapper>> _comWrapperCache = new ConcurrentDictionary<int, WeakReference<ISafeComWrapper>>();
12+
13+
14+
public void Add(ISafeComWrapper comWrapper)
15+
{
16+
if (comWrapper != null)
17+
{
18+
_comWrapperCache.AddOrUpdate(
19+
GetComWrapperObjectHashCode(comWrapper),
20+
key => new WeakReference<ISafeComWrapper>(comWrapper),
21+
(key, value) => new WeakReference<ISafeComWrapper>(comWrapper));
22+
}
23+
24+
}
25+
26+
//We do not use GetHashCode because subclasses of SafeComWrapper<T> overwrite this method
27+
//and we need to distinguish between individual instances.
28+
private int GetComWrapperObjectHashCode(ISafeComWrapper comWrapper)
29+
{
30+
return RuntimeHelpers.GetHashCode(comWrapper);
31+
}
32+
33+
public bool TryRemove(ISafeComWrapper comWrapper)
34+
{
35+
return !_disposed && comWrapper != null && _comWrapperCache.TryRemove(GetComWrapperObjectHashCode(comWrapper), out _);
36+
}
37+
38+
private bool _disposed;
39+
public void Dispose()
40+
{
41+
if (_disposed)
42+
{
43+
return;
44+
}
45+
46+
_disposed = true;
47+
48+
foreach (var weakReference in _comWrapperCache.Values)
49+
{
50+
if(weakReference.TryGetTarget(out var comWrapper))
51+
{
52+
comWrapper.Dispose();
53+
}
54+
}
55+
56+
_comWrapperCache.Clear();
57+
}
58+
}
59+
}

‎Rubberduck.VBEEditor/Rubberduck.VBEditor.csproj

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,8 @@
128128
<Reference Include="System.Windows.Forms" />
129129
</ItemGroup>
130130
<ItemGroup>
131+
<Compile Include="ComManagement\WeakComSafe.cs" />
132+
<Compile Include="ComManagement\ComSafeManager.cs" />
131133
<Compile Include="ComManagement\IProjectsProvider.cs" />
132134
<Compile Include="ComManagement\IProjectsRepository.cs" />
133135
<Compile Include="ComManagement\ProjectsRepository.cs" />
@@ -136,7 +138,7 @@
136138
<Compile Include="ComManagement\TypeLibs\TypeLibsSupport.cs" />
137139
<Compile Include="ComManagement\TypeLibs\TypeLibsAPI.cs" />
138140
<Compile Include="ComManagement\ReferenceEqualityComparer.cs" />
139-
<Compile Include="ComManagement\ComSafe.cs" />
141+
<Compile Include="ComManagement\StrongComSafe.cs" />
140142
<Compile Include="ComManagement\IComSafe.cs" />
141143
<Compile Include="Events\ComponentEventArgs.cs" />
142144
<Compile Include="Events\ComponentRenamedEventArgs.cs" />

‎RubberduckTests/VBEditor/ComSafeManagerTests.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ public class ComSafeManagerTests
99
[Test()]
1010
public void ComSafeReturnedOnSecondIvokationOfGetCurrentComSafeIsTheSame()
1111
{
12-
ComSafeManager.ResetComSafe(); //Resetting to get a claen start.
12+
ComSafeManager.DisposeAndResetComSafe(); //Resetting to get a claen start.
1313

1414
var comSafe1 = ComSafeManager.GetCurrentComSafe();
1515
var comSafe2 = ComSafeManager.GetCurrentComSafe();
@@ -20,10 +20,10 @@ public void ComSafeReturnedOnSecondIvokationOfGetCurrentComSafeIsTheSame()
2020
[Test()]
2121
public void AfterCallingResetComSafeGetCurrentComSafeReturnsDifferentSafe()
2222
{
23-
ComSafeManager.ResetComSafe(); //Resetting to get a claen start.
23+
ComSafeManager.DisposeAndResetComSafe(); //Resetting to get a claen start.
2424

2525
var comSafe1 = ComSafeManager.GetCurrentComSafe();
26-
ComSafeManager.ResetComSafe();
26+
ComSafeManager.DisposeAndResetComSafe();
2727
var comSafe2 = ComSafeManager.GetCurrentComSafe();
2828

2929
Assert.AreNotSame(comSafe1, comSafe2);

‎RubberduckTests/VBEditor/ComSafeTests.cs

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@
66
namespace RubberduckTests.VBEditor
77
{
88
[TestFixture()]
9-
public class ComSafeTests
9+
public abstract class ComSafeTestBase
1010
{
11+
protected abstract IComSafe TestComSafe();
12+
1113
[Test()]
1214
public void TryRemoveOnNewComSafe_ReturnsFalse()
1315
{
14-
var comSafe = new ComSafe();
16+
var comSafe = TestComSafe();
1517
var testComWrapper = new Mock<ISafeComWrapper>().Object;
1618
var result = comSafe.TryRemove(testComWrapper);
1719

@@ -21,7 +23,7 @@ public void TryRemoveOnNewComSafe_ReturnsFalse()
2123
[Test()]
2224
public void TryRemoveWithItemPreviouslyAddedToComSafe_ReturnsTrue()
2325
{
24-
var comSafe = new ComSafe();
26+
var comSafe = TestComSafe();
2527
var testComWrapper = new Mock<ISafeComWrapper>().Object;
2628
comSafe.Add(testComWrapper);
2729
var result = comSafe.TryRemove(testComWrapper);
@@ -32,7 +34,7 @@ public void TryRemoveWithItemPreviouslyAddedToComSafe_ReturnsTrue()
3234
[Test()]
3335
public void TryRemoveWithOtherItemPreviouslyAddedToComSafe_ReturnsFalse()
3436
{
35-
var comSafe = new ComSafe();
37+
var comSafe = TestComSafe();
3638
var testComWrapper = new Mock<ISafeComWrapper>().Object;
3739
var otherTestComWrapper = new Mock<ISafeComWrapper>().Object;
3840
comSafe.Add(otherTestComWrapper);
@@ -44,7 +46,7 @@ public void TryRemoveWithOtherItemPreviouslyAddedToComSafe_ReturnsFalse()
4446
[Test()]
4547
public void TryRemoveWithItemAndOtherItemPreviouslyAddedToComSafe_ReturnsTrue()
4648
{
47-
var comSafe = new ComSafe();
49+
var comSafe = TestComSafe();
4850
var testComWrapper = new Mock<ISafeComWrapper>().Object;
4951
var otherTestComWrapper = new Mock<ISafeComWrapper>().Object;
5052
comSafe.Add(otherTestComWrapper);
@@ -57,7 +59,7 @@ public void TryRemoveWithItemAndOtherItemPreviouslyAddedToComSafe_ReturnsTrue()
5759
[Test()]
5860
public void SecondTryRemoveWithItemPreviouslyAddedToComSafe_ReturnsFalse()
5961
{
60-
var comSafe = new ComSafe();
62+
var comSafe = TestComSafe();
6163
var testComWrapper = new Mock<ISafeComWrapper>().Object;
6264
comSafe.Add(testComWrapper);
6365
comSafe.TryRemove(testComWrapper);
@@ -69,7 +71,7 @@ public void SecondTryRemoveWithItemPreviouslyAddedToComSafe_ReturnsFalse()
6971
[Test()]
7072
public void SecondTryRemoveWithItemPreviouslyAddedToComSafeTwice_ReturnsFalse()
7173
{
72-
var comSafe = new ComSafe();
74+
var comSafe = TestComSafe();
7375
var testComWrapper = new Mock<ISafeComWrapper>().Object;
7476
comSafe.Add(testComWrapper);
7577
comSafe.Add(testComWrapper);
@@ -82,7 +84,7 @@ public void SecondTryRemoveWithItemPreviouslyAddedToComSafeTwice_ReturnsFalse()
8284
[Test()]
8385
public void AddedSafeComWrapperGetsDisposedOnDisposalOfComSafe()
8486
{
85-
var comSafe = new ComSafe();
87+
var comSafe = TestComSafe();
8688
var mock = new Mock<ISafeComWrapper>();
8789
mock.Setup(wrapper => wrapper.Dispose());
8890

@@ -96,7 +98,7 @@ public void AddedSafeComWrapperGetsDisposedOnDisposalOfComSafe()
9698
[Test()]
9799
public void SafeComWrapperAddedTwiceGetsDisposedOnceOnDisposalOfComSafe()
98100
{
99-
var comSafe = new ComSafe();
101+
var comSafe = TestComSafe();
100102
var mock = new Mock<ISafeComWrapper>();
101103
mock.Setup(wrapper => wrapper.Dispose());
102104

@@ -111,7 +113,7 @@ public void SafeComWrapperAddedTwiceGetsDisposedOnceOnDisposalOfComSafe()
111113
[Test()]
112114
public void RemovedSafeComWrapperDoesNotGetDisposedOnDisposalOfComSafe()
113115
{
114-
var comSafe = new ComSafe();
116+
var comSafe = TestComSafe();
115117
var mock = new Mock<ISafeComWrapper>();
116118
mock.Setup(wrapper => wrapper.Dispose());
117119

@@ -126,7 +128,7 @@ public void RemovedSafeComWrapperDoesNotGetDisposedOnDisposalOfComSafe()
126128
[Test()]
127129
public void SafeComWrapperRemovedAfterHavingBeenAddedTwiceDoesNotGetDisposedOnDisposalOfComSafe()
128130
{
129-
var comSafe = new ComSafe();
131+
var comSafe = TestComSafe();
130132
var mock = new Mock<ISafeComWrapper>();
131133
mock.Setup(wrapper => wrapper.Dispose());
132134

@@ -142,7 +144,7 @@ public void SafeComWrapperRemovedAfterHavingBeenAddedTwiceDoesNotGetDisposedOnDi
142144
[Test()]
143145
public void AddedSafeComWrapperGetsDisposedOnDisposalOfAfterOtherItemGotRemovedComSafe()
144146
{
145-
var comSafe = new ComSafe();
147+
var comSafe = TestComSafe();
146148
var mock = new Mock<ISafeComWrapper>();
147149
mock.Setup(wrapper => wrapper.Dispose());
148150

@@ -159,7 +161,7 @@ public void AddedSafeComWrapperGetsDisposedOnDisposalOfAfterOtherItemGotRemovedC
159161
[Test()]
160162
public void AddedSafeComWrapperDoesNotGetDisposedAgainOnSecondDisposalOfComSafe()
161163
{
162-
var comSafe = new ComSafe();
164+
var comSafe = TestComSafe();
163165
var mock = new Mock<ISafeComWrapper>();
164166
mock.Setup(wrapper => wrapper.Dispose());
165167

@@ -174,7 +176,7 @@ public void AddedSafeComWrapperDoesNotGetDisposedAgainOnSecondDisposalOfComSafe(
174176
[Test()]
175177
public void AfterDisposalTryRemoveReturnsFalseForAddedItem()
176178
{
177-
var comSafe = new ComSafe();
179+
var comSafe = TestComSafe();
178180
var mock = new Mock<ISafeComWrapper>();
179181
mock.Setup(wrapper => wrapper.Dispose());
180182

@@ -185,5 +187,23 @@ public void AfterDisposalTryRemoveReturnsFalseForAddedItem()
185187

186188
Assert.IsFalse(result);
187189
}
190+
191+
[TestFixture()]
192+
public class StrongComSafeTests : ComSafeTestBase
193+
{
194+
protected override IComSafe TestComSafe()
195+
{
196+
return new StrongComSafe();
197+
}
198+
}
199+
200+
[TestFixture()]
201+
public class WeakComSafeTests : ComSafeTestBase
202+
{
203+
protected override IComSafe TestComSafe()
204+
{
205+
return new WeakComSafe();
206+
}
207+
}
188208
}
189209
}

0 commit comments

Comments
 (0)