Partager via

IUnkown::Release() implementation

  • Article

Mike Stall talks about “Don’t do complicated work in Release()”. He has a sample implementation for IUnkown::Release().

 

<quote>

 

An object’s destructor has the same caveats if Release() is often implemented as:

 

InterlockedDecrement(&m_ref)

if (m_ref == 0) { delete this; }

</quote>

 

There is a bug in this implementation, that m_ref is accessed after InterlockedDecrement. The problem is the object may have been deleted after InterlockedDecrement. Thus access violation exception may occur when m_ref is accessd after the memory has been reclaimed.

 

Imagine the object’s ref count is 2. There are two threads calling Release() simultaneously.

 

Thread 1 Thread 2

 

InterlockedDecrement(&m_ref);

//m_ref now is 1

           

                                                                                    InterlockedDecrement(&m_ref);

                                                                                    // m_ref now is 0

                                                                                    if (m_ref==0) {delete this;}

                                                                                    // now the object is gone

 

// access violation here, since the object is gone

if (m_ref==0) {delete this;}

 

The right implementation is

 

if (!InterlockedDecrement(&m_ref)) {delete this;}

 

We have found a few occurrences of this pattern in CLR’s code base. This bug is revealed in our internal ASP.NET stress. We have fixed all of them in CLR’s codebase since.

 

Devil is in the details.

Comments

  • Anonymous
    February 27, 2005
    You guys might want to check the RCW cookie cutter template code used in ActiveX imports for that as well =)

    I've been seeing quite a few hard to pin down thread deadlocks with code that uses the IE control and I think it must be something like this. I ended up doing a manual AddRef() on the control and having to live with one leaked instance to avoid an app getting frozen to heck.
  • Anonymous
    February 28, 2005
    The comment has been removed
  • Anonymous
    February 28, 2005
    The comment has been removed
  • Anonymous
    March 01, 2005
    Though I haven't read Rotor's code, I have read/debugged enough other code from MS with hand implemented, incomplete, inconsistent and often buggy implementations of COM contracts.

    Reusing, extending those objects is a big pain. Imagine the frustation when you debug into someone else's QI and find out that the author of the QI forgot to add a case for IUnknown! I can understand not using STL, but knowing little bit about ATL (and the claims of newer optimizing compilers) I have difficulty in understanding the resons not to use it.

    Even if ATL is not used, I would expect there to be some common internal framework used to implement such boilerplate code. Please help me understand why reinventing the wheel makes sense here?

  • Anonymous
    March 01, 2005
    You can never write code of this form:

    InterlockedDecrement(&m_ref )
    if (m_ref == 0) { delete this; }

    You might just as well have written

    m_ref --;
    if (m_ref == 0) { delete this; }

    By not using the return of the InterlockedDecrement it's possible that two threads will see the zero value and the destructor will be run twice, possibly in an overlapped fashion. Accessing m_ref after the operation is just the tip of an iceberg of problems.
  • Anonymous
    March 05, 2005
    Did tyis solution worked?
    D. Orbach<br/>
    <a href="http://www.javasight.com"><b>javasight - java news & books</b></a>
  • Anonymous
    March 05, 2005
    Did tyis solution worked?
    D. Orbach
    http://www.javasight.com javasight - java news & books
  • Anonymous
    May 16, 2005
    RePost:
    http://www.yeyan.cn/Programming/IUnkownReleaseimplementation.aspx