What's wrong with this code, part 14 - the answers

Yesterday's post was a classic example of Joel Spolsky's Law of Leaky Abstractions.

Why?  Well, because it was an example of conflicting contracts.

In COM, the contract for an API is defined  by the APIs interface.  In this case, it was:

[     object,     uuid("0A0DDEDC-C422-4BB3-9869-4FED020B66C5"),     pointer_default(unique) ] __interface IFoo : IUnknown{    HRESULT GetFooConfig([out] struct FooConfig **ReturnedFooConfig);};

A straightforward contract - there's a function named "GetFooConfig" that fills in a pointer with a pointer to a FooConfig structure.  Nothing exciting there.

In this case, the author of the interface chose to return a pointer to a global variable for the output value.  Again, nothing exciting, there's no reason that it wouldn't work - if the _GlobalFooConfig variable is constant, there aren't even multi-threaded access issues (it could be put in a read-only section, for example).

But there's a problem.  The thing is that COM (actually the RPC runtime library, but it's easier to blame COM) has an additional requirement for [out] pointers.  This requirement is that if the type of an [out] parameter isn't a scalar quantity (in other words if it's a structure or anything more complicated than a int or float), then the memory pointed to by the [out] parameter needs to be allocated either by MIDL_user_allocate (for RPC) or CoTaskMemAlloc (for COM).

This is a leaky abstraction - the abstract interface says nothing about this additional requirement, so the COM requirement leaks through to the implementation.  And what's worse, the leak doesn't show up until the RPC marshaller gets involved in the process - until that happens, the interface works perfectly.

So why did this work until the new tests were rolled out?  Well, it was because up until then, the COM object had always been executed in-proc.  But the new test case instantiated the COM object out-of-proc and the RPC runtime library tried to marshal the code and...  Boom.

Kudos:

First off, bao caught a stupid mistake in GetFooConfig() that was unintentional.

ThaleseC correctly called out the fact that we were returning a static variable, but the pieces didn't get fully put together until vrk caught the marshaling issue.

Ralf's comment about not checking for a null ReturnedFooConfig is interesting - the contract for GetFooConfig specifies that the ReturnedFooConfig parameter isn't optional (because it's flagged as [out]).  The RPC runtime guarantees that ReturnedFooConfig is always valid.  And in-proc, crashing on a null parameter is probably as useful as checking for a null pointer - that way programming errors on the part of the client of the interface get caught quickly.

Other comments: Alex pointed out that this wasn't pure ansi C++ because it used the Microsoft C++ annotation extensions.  He's right.

Mike pointed out that my stylistic choice of _Xxx for global variables (and member variables) conflicts with the C++ standard - he's right, they are reserved.

rederick Slijkerman pointed out that CFoo should derive from IUnknown.  This is true, and it happens already, since IFoo derives from IUnknown.  Adding an explicit interface isn't necessary (and can potentially lead to ambiguity)

Finally, mirobin commented on the fact that the call returns a pointer to a data structure - as I pointed out, this isn't necessarily a problem, assuming that the rules for the FooConfig are clearly understood.

 

On a related note, I'm insanely busy right now so I suspect I'll be rather short on new posts for the next week or so...

Comments

• Anonymous
  July 08, 2005
  "This requirement is that if the type of an [out] parameter isn't a scalar quantity (in other words if it's a structure or anything more complicated than a int or float), then the memory pointed to by the [out] parameter needs to be allocated either by MIDL_user_allocate (for RPC) or CoTaskMemAlloc (for COM)."
  
  I'd like to see where this is specified - is it really universally true?
• Anonymous
  July 08, 2005
  Thomas,
  
  See the Memory Management Rules topic in MSDN. Regarding "out" parameters, it says: "Must be allocated by the one called; freed by the caller using the standard COM task memory allocator. Refer to The OLE Memory Allocator for more information."
  
  http://msdn.microsoft.com/library/en-us/com/html/769127a1-1a14-4ed4-9d38-7cf3e571b661.asp
• Anonymous
  July 08, 2005
  The comment has been removed
• Anonymous
  July 08, 2005
  What you are asking for isn't a true randomizer. It is a randomizer with memory. Thus any two randomized events are not independent.
• Anonymous
  July 08, 2005
  I used to suspect that WMP 7 reverse engineered winamp's old random - they're both so BAD. Most likely related to rand(x) not being nearly as random as most programmers think. I'm sure WMP 9 just reimplemented it; various random algorithms are available online. You could always send a testy email linking to a few of them to Creative.
  
  Sometimes randoms with tails, so that once a song is played it won't be within, say, 30 songs or the entire playlist, are nifty, and random-based-on-rating is way cool, but strict random always works as long as it really is.
  
  That said, I think your Zen just likes the Doors. xD
• Anonymous
  July 08, 2005
  So that's why I should call that CoTaskMemFree() function for the returned value of SHBrowseForFolder().
  
  I was wondering why the memory if not freed by the function in the same DLL...
• Anonymous
  July 08, 2005
  The implementation of "random" playlists in most software annoys me to no end. I want it to play every song in my playlist in a random order, without repeating a song until it has gone through the entire list.
  
  Screensavers that show pictures are also nefarious culprits.
  
  Nobody ever seems to get this right. Yet it is so easy to code for its rediculous ... All you do is start off with an ordered array and iterate over it, each entry with a random entry. You end up with a very well randomized list, and you don't even have to iterate over the entire thing to shuffle it if you're sneaky ...
  
  Bleh.
  
  /end rant on playlists
• Anonymous
  July 09, 2005
  Even with a perfect random number generator, it is still won't work the way you think.
  
  For example, if I have 10 songs using a true random number generator without memory, there is a 10% chance that with just two songs being played, the first one will be repeated.
  
  The probability of having a non-repeating sequence of n for m songs is (m!/(m-n)!)/m^n.
  
  Just too geek out, here is the probability table:
  
  Songs, Play#, Prob
  100 1 100.00%
  100 2 99.00%
  100 3 97.02%
  100 4 94.11%
  100 5 90.35%
  100 6 85.83%
  100 7 80.68%
  100 8 75.03%
  100 9 69.03%
  100 10 62.82%
  100 11 56.53%
  100 12 50.32%
  100 13 44.28%
  100 14 38.52%
  100 15 33.13%
  100 16 28.16%
  100 17 23.65%
  100 18 19.63%
  100 19 16.10%
  100 20 13.04%
  
  As you can see, if you have 100 songs, there is a small chance of no repeats just 20 songs in.
• Anonymous
  July 09, 2005
  Mirobin, you don't want random play, you want a shuffled playlist and linear play. Almost every media player has a shuffle playlist button; though some need a few shots to mix it up enough. I'm not sure if any will reshuffle when you hit the end of the playlist, if that even matters to you; you could always do it by hand. (Not all portable players support shuffle, you'd have to import their playlists pre-shuffled each time.)
  
  You're so frustrated because you've been using the wrong function. ^.~