Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

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

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

  • Comments 16

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...

  • "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?
  • 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
  • Well Larry, since your going to be really busy. I have something for you to mull over one of those things to stick in your head. Maybe someday you can blog the answer. Windows Media Player 7 and before. When you had a few thousand mp3's or any audio and you put it on random play, the random play was terrible. You often got many repeats and some songs would never play or if they did it was once in a blue moon. Media player 9 and 10 Random play is quite good it apears to be random and it may be several days before I hear the same song. Also pass the word, I know something changed between 7 and 9 on the randomization and I very much aprove so someone changed something so tell them thanks for me.

    Now Why I ask? I also have a creative labs Zen Portable device. Sincs with media player and everything fine, a very excellent device. However it's random play is horrid. I have 186 hours of music on it, I have about 1 hour of The Doors on it, for some reason I get a Doors song every hour. While I like the Doors, day after day after day you need something different, there are also songs on there I have loaded and I have never heard unless I go in and specifically choose to play them. The other wierd thing is I got tired of The Doors every single hour so I removed them from the device. A few months went by I put The Doors back on and again a song by them every hour. This is the same experience I used to have with Media player 7 years ago, drove me nuts to the point I was going to just use the media player com component and write my own media player and use randomization based on when was last time played and so on. So anyway what is making the randomization in Media player now so much better that the randomization back in 7 can you share that with us? Can you share that with Creative? The other reason I ask is not just in music but true very good randomization would be very usefull to know in a variety of things.
  • What you are asking for isn't a true randomizer. It is a randomizer with memory. Thus any two randomized events are not independent.
  • 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
  • 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...
  • 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
  • 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.
  • 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. ^.~
Page 1 of 1 (9 items)