12

We have a third party library that internally uses a SafeHandle to an unmanaged resource. In some error cases it is necessary to dispose of the object and recreate it. But, there is a bug in the dispose implementation that prevents the Handle from being closed in a subset of these cases. Which prevents new objects from being successfully created until its finalizer runs.

Two solutions (both evil) have been proposed to deal with this until we can get the third party code fixed:

  1. Run a GC.Collect to get the finalizer to run and clean up the object

  2. Use reflection to get at the Handle and close it if dispose fails to do so

which of these is less evil and why? Is there some other method that we haven't considered that is less evil than either of these?

Cody Gray - on strike
  • 239,200
  • 50
  • 490
  • 574
Yaur
  • 7,333
  • 1
  • 25
  • 36

5 Answers5

14

I'm in favour of private reflection. It's a localized bug, so the solution should be local too. And it's much clearer that what your code intends to do. And you probably can add some tests that notice once the bug has been fixed. So the hack can easily be removed once it's not needed anymore.

...
thirdPartyObject.Dispose();
ThirdPartyDisposeBugWorkaround(thirdPartyObject);
...

void ThirdPartyDisposeBugWorkaround(ThirdPartyClass thirdPartyObject)
{
   //Do private reflection here
}

Forcing a GC on the other hand has a global effect. And there are many reasons(most of them bad) for interfering with the GC. It's much less obvious what your code does. So the call might be kept even once the bug has been fixed.

Old New Thing: Don't use global state to manage a local problem

CodesInChaos
  • 106,488
  • 23
  • 218
  • 262
  • 3
    A thousand times agreed. Calling `GC.Collect` is almost always the wrong thing to do. And bonus points for linking to Raymond's blog. But whatever you decide, the important thing is to **document it extensively**. – Cody Gray - on strike May 21 '11 at 14:59
5

I would go with reflection, but make sure you have error handling around it that makes it explicitly clear what's wrong, keeping in mind the error might not get triggered until years from now and your dev team could have turned over and nobody remembers this wacky hack.

try
{
   .. hacky reflection ..
}
catch(Exception ex)
{
    throw new Exception("Reflection on private field 'Xyz' of 3rd Party Component 'Abc' failed.  Was 'Abc' updated? Reflection is used due to bug in 'Dispose' implementation.", ex);
}
Samuel Neff
  • 73,278
  • 17
  • 138
  • 182
  • I'd probably use `Debug.Assert`. And if I were going to throw an exception, I *definitely* wouldn't throw `System.Exception`. Perhaps `InvalidOperationException`? – Cody Gray - on strike May 21 '11 at 14:55
  • @Cody Gray, internally we use a helper class we call `ExceptionUtil.Rethrow` which makes a new exception that's the same type as the original, useful when just adding more information. I certainly wouldn't want to use `Debug.Assert` because this is an exception that will occur in production if it's possible to update the 3rd party library separately from the app (which of course is a horrible idea, but people do stupid things). – Samuel Neff May 21 '11 at 15:40
  • I'm just not sure that you want to throw an exception in the case of a memory leak. A debug assertion seems good enough. Yes, it's a horrible idea. If someone does that, they've created a bug. Better yet, just prevent them from doing so in a first place: tie builds of your app to a specific version of the third-party DLL. A good idea anyway, since you're relying on explicitly undocumented behavior. And, I don't really know what your helper class has to do with anything? I mean, I guess I feel better that you don't write wrong code throwing the base `Exception` class... ;-) – Cody Gray - on strike May 21 '11 at 15:52
  • @Cody Gray, this is not going to be an exception when a memory leak occurs. When you use reflection to call a property that doesn't exist, you'll get an exception. That point is to catch it and rethrow with more information. The more I think about it though, you really can just avoid the exception in the first place with a null check--try to get the field, if it isn't there, then throw an exception or whatever is appropriate for the application and architecture. Either way, the concept is the same. – Samuel Neff May 21 '11 at 23:05
3

First pick the one that works. If they both work pick the one that has the least amount of impact on the system. Gc.Collect is more of a hammer across the whole application. Where as your reflection code is brittle but should have a very small impact.

Aaron Fischer
  • 20,853
  • 18
  • 75
  • 116
  • GC.Collect definitely works (though this is obviously not guaranteed) which means that the reflection code should work since GC.Collect isn't going to do anything magical except cause the finalizer to run. – Yaur May 21 '11 at 15:14
2

If it is not marked as sealed, you could inherit from it and implement your own dispose. As for the reflection vs GC, I would definately use reflection. Like others have said, the GC may not work as expected. It could do a collection iteration but not actually release your handle.

I would like to note: if something else still has a reference to this SafeHandle, and you release it, you could easily introduce other bugs into your system.

Charles Lambert
  • 5,042
  • 26
  • 47
  • I think were safe based on looking at a big chunk of the code in question in reflector... but am aware of that risk. I don't think a dispose override would work since the field in question is private, not protected. – Yaur May 21 '11 at 15:08
  • 1
    I was intending for you to use inheritance as the place to put your reflection code. That way your unique situation would be abstracted to the place it belongs. Which is during the dispose process. – Charles Lambert May 21 '11 at 15:16
  • thats a really good idea, and i'll have to figure out how to make that work. The only complexity is that "dispose" is really a non-virtual member function that acts like dispose... which is pretty minor in the context of the overall problem. – Yaur May 21 '11 at 15:30
0

Taking the "CodeInChaos" argument forward why don't you call for collection on specific generation.

GC.GetGeneration(Object obj) will return the generation that object is in and GC.Collect(Int32 gen).

Ex:

Int32 generation = GC.GetGeneration(theObject);
theObject = null;
GC.Collect(generation);
GC.WaitForPendingFinalizers();
GC.Collect(generation); // this is req because first collect puts thisObject on freachable queue not // garbaged yet.
Vijay Sirigiri
  • 4,653
  • 29
  • 31
  • No, **definitely don't do this**. This is in complete contradiction to the argument CodeInChaos made. – Cody Gray - on strike May 21 '11 at 15:23
  • Under most circumstances yes. But in his case this (GC control) is one of the option. I was referring to "Its a localized bug" statement by CodeInChaos.. so instead of doing global Collect I was directing towards Collect on the specific generation. Calling GC.Collect() and overloads is not advised if the reason is to improve the performace but it is not such a bad option in exceptional cases like his which should occur only when the code is ready to dispose the object. – Vijay Sirigiri May 21 '11 at 16:49
  • 1
    No, it's still ill-advised. This is still a global solution to a local problem. It's just a global solution that impacts fewer global objects. This is still contradictory to CodeInChaos's advice. – Cody Gray - on strike May 21 '11 at 16:50