3

I saw this sample code in SO that was saying one practice is bad and the other one is good. But I don't understand why? As a matter of fact I am getting that famous RCW COM object error and that post was saying this could be a reason. 

public class SomeClass
{
    private Interop.ComObjectWrapper comObject;
    private event ComEventHandler comEventHandler;

    public SomeClass()
    {
        comObject = new Interop.ComObjectWrapper();

        // NO - BAD!
        comObject.SomeEvent += new ComEventHandler(EventCallback);

        // YES - GOOD!
        comEventHandler = new ComEventHandler(EventCallback);
        comObject.SomeEvent += comEventHandler
    }

    public void EventCallback()
    {
        // DO WORK
    }

}

Edit: here is the link to the source: COM object that has been separated from its underlying RCW cannot be used

Community
  • 1
  • 1
Bohn
  • 26,091
  • 61
  • 167
  • 254
  • Could you link to the sample code in question? – JoshVarty Nov 26 '12 at 15:17
  • Who is asserting that one is good and one is bad; could you link to where they stated this, if it's public and online? – Servy Nov 26 '12 at 15:18
  • I would just do: `comObject.SomeEvent += EventCallback;`. The `new ComEventHandler` is implied and added automatically by the C# compiler. – Servy Nov 26 '12 at 15:20
  • @Servy: Updated the question with the link. http://stackoverflow.com/questions/1567017/com-object-that-has-been-separated-from-its-underlying-rcw-cannot-be-used – Bohn Nov 26 '12 at 15:21
  • @BDotA He explains why they're different, and why one is good and one is not. What problem do you have with that explanation? – Servy Nov 26 '12 at 15:22
  • @Servy : well is what he is suggesting correct? if yes : can you please explain it a little in more detail ? and does it only apply to COM event handlers or even for normal .NET object we should do the same? – Bohn Nov 26 '12 at 15:24
  • @BDotA I can tell you for sure this doesn't make sense for the majority of event handlers; normally doing what he's suggesting would be unneeded bloat. Not having done any COM work I couldn't say if it's needed in that instance. In any case, I'd suggest commenting on that answer to ask him to elaborate. – Servy Nov 26 '12 at 15:26
  • check out this msdn for if it is needed for non-COM objects (it's not): http://msdn.microsoft.com/en-us/library/ms366768(v=vs.80).aspx – slawekwin Nov 26 '12 at 15:35
  • @Servy: I'm not actually sure, that holding references to the delegate in the field will solve an issue with a weak references. Even holding references to the delegate in the comEventHandler field will not not prevent the whole object graph from garbage collection. We still should add additional root for the whole object of type SomeClass to prevent the whole graph from GC. – Sergey Teplyakov Nov 26 '12 at 16:06
  • It's confusing that the private member `comEventHandler` is an `event`. I think it would be easier to read the code (and simpler) if `comEventHandler` was just a **field**. Maybe even a `readonly` field. – Jeppe Stig Nielsen Nov 26 '12 at 16:11
  • @SergeyTeplyakov : "We still should add additional root for the whole object of type SomeClass to prevent the whole graph from GC" .. Thanks, could you please add some code sample of what you think we still need to add ? – Bohn Nov 26 '12 at 16:50
  • 1
    @BDotA see my answer for more details. – Sergey Teplyakov Nov 27 '12 at 10:16

1 Answers1

4

I think those two code snippets are the same and we don't have any issues with strong/weak references here.

Background

First of all, if our Interop.ComObjectWrapper provides CLR event (i.e. event that stores event handlers in delegates) we'll definitely got a strong reference from ComObjectWrapper to our object.

Any delegate contains two parts: Target of type object and Method Pointer to the particular method. If Target is null than callback points to the static method.

Its impossible to have a delegate with Target of type WeakReference. There is so called Weak Event Pattern but it implements on top of EventManager instead of plain delegates.

Storing event handler in the field will not help. Part 1

Internal event implementation means that after subscribing to the event:

comObject.SomeEvent += EventCallback;

comObject object implicitely holds an strong reference to the SomeClass object. And this true regardless of what kind of subscribing technique your are using and whether ComObject is a COM object wrapper or not.

Subscribing to the event adds implicit dependency between two objects in terms of lifetime. That's why the most common memory leak in .NET world caused by subscribing to events of the long-lived objects. Event subscriber will not die till event holder accessible in the application.

Storing event handler in the field will not help. Part 2

But event if my assumption is not true and ComObjectWrapper provides some notion of Weak Event Pattern, saving event handler in the field will not help any way.

Lets recap what event keyword mean:

private event ComEventHandler comEventHandler;
... 
comEventHandler = new ComEventHandler(EventCallback);

Saving callback in current field (and basically we can treat private events as a simple delegate fields) will not change existing behavior.

We already know that delegate is a simple object that stores reference to the Target object (i.e. SomeClass object) and a method (i.e. public void EventCallBack()). This means that storing additional delegate in field adds additional reference to the SomeClass from the SomeClass itself.

Basically, storing event handler in the field semantically equivalent to storing additional reference in SomeClass:

private SomeClass someClass;

public SomeClaas() { // This is basically the same as storing delegate // in the comEventHandler field someClass = this; }

Storing strong reference in the SomeClass will not prolong lifetime of the current object. And this means that if ComObjectWrapper will not hold a strong reference to the SomeClass object storing event handler in the comEventHandler will not prolong SomeClass's lifetime and will not prevent SomeClass from garbage collection.

Conclusion

Storing event handler in the private field will not prolong object's lifetime and will not prevent it from garbage collection anyway.

That's why there is no difference between following code snippets in terms of object lifetime:

    // GOOD!
    comObject.SomeEvent += new ComEventHandler(EventCallback);

    // EVEN BETTER!
    comObject.SomeEvent += EventCallback;

    // NOT GOOD, BECAUSE WAN'T HELP!
    comEventHandler = new ComEventHandler(EventCallback);
    comObject.SomeEvent += comEventHandler
Sergey Teplyakov
  • 11,477
  • 34
  • 49
  • thank you so much Sergey for the answer. Learned something new. So back to the main problem, you mentioned :"Event subscriber will not die till event holder accessible in the application. " Could you please suggest some debugging techniques or some sort of solution to this problem on how to locate hte problem and solve it? – Bohn Nov 27 '12 at 17:04
  • You should take a look at other comments mentioned in this post - http://stackoverflow.com/questions/1567017/com-object-that-has-been-separated-from-its-underlying-rcw-cannot-be-used. To check whether you have an issue with object lifetime you can add instance of this object to the static list of object: private static List _neverDyingList = new List(); // in the method _neverDyingList.Add(this); If this don't help that would mean that you have some other issue like Appartment issue or something else. – Sergey Teplyakov Nov 27 '12 at 18:04