Derik Whittaker

Syndication

News


Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?

I am in the middle of building out some wcf endpoints to support the Dimecasts.net WP7 application I am going to publish.  The code below is some of my main logic which helps grab the episodes.  Because the data for Dimecasts is not very volatile I am going to do some basic (aka primitive) caching. 

What I am looking to do is save the amount of data which needs to be gathered from the db.  The intent of the code below is to get all data from cache which is greater then the last known episode which is cached on the phone.  If data is found in the cache we will adjust the ‘last known episode’ in order to again reduce the db hit.  However, there is a slight bug.

Who can spot the bug?

        public IList<EpisodeModel> FetchEpisodes( Int32 lastKnownEpisodeId )
        {
            var episodes = new List();
            var cachedEpisodes = Cache.FetchEpisodes()
                .Where( x => x.EpisodeNumber > lastKnownEpisodeId )
                .OrderBy( x => x.Id );

            if ( cachedEpisodes.Count() != 0 )
            {
                // reset the last known id in order to reset the reset
                lastKnownEpisodeId = cachedEpisodes.LastOrDefault().Id;

                episodes.AddRange( cachedEpisodes );
            }

            // more code which does not matter here.....

            return episodes;
        }

If you know the issue post it in the comments.  If it is not ‘solved’ I will post an update with the solution and the reason.

Till next time,


Posted 01-21-2011 5:10 PM by Derik Whittaker
Filed under: ,

[Advertisement]

Comments

frederikm wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-21-2011 6:30 PM

lazy evaluation of your cachedEpisodes var will make it get executed twice (lines 11 & 13)

make sure that you add a ToList() at the end of the cachedEpisodes  declaration to solve the issue :)

jesus delatorre wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-21-2011 6:33 PM

It seems like you are updating a local variable (lastKnownEpisodeId ) when you should be updated some sort of global variable.

But your bug is that you need an OrderByDescending not an OrderBy

Darren Kopp wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-21-2011 6:47 PM

linq uses deferred execution (because it uses lazy enumerable). your query uses lastKnownEpisodeId in the query, so it's capturing it in the closure.

when you hit a query where count != 0, you update the lastKnownEpisodeId's value, so when you go to add them to the episodes, it will re-evaluate the query and filter the cache using the new value of lastKnownEpisodeId, in which i'm guessing no episodes match. not sure why you set the lastKnownEpisodeId though (unless you use it somewhere in the code you snipped)

this isn't a real "linq issue" per say as it is more of a closure issue.  a more detailed dive into the issue here is explained by eric lippert on his blog:

blogs.msdn.com/.../closing-over-the-loop-variable-considered-harmful.aspx

blogs.msdn.com/.../closing-over-the-loop-variable-part-two.aspx

Derik Whittaker wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-21-2011 6:47 PM

@Frederikm,

Ding, Ding, Ding you are right :)

NicoGranelli wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-21-2011 8:28 PM

Ooh, it will execute the query again, coz you changed the value of the parameter. Didn't know that :)

BTW, I think updating the value of a parameter is a code smell. You should declare a local variable and use that instead. It wont fix the code, but the smell it's nasty :)

Alexey wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-21-2011 10:17 PM

It's nothing to do with LINQ. The developer who wrote this probably doesn't know what closures are and how they work.

So the problem is the access to modified closure.

LINQ still rocks, just ask this dev to learn basics :)

Eber Irigoyen wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-22-2011 11:35 AM

so the bug is that you are updating lastKnownEpisodeId, but you could use .Any instead of .Count

Chris Tavares wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-22-2011 5:44 PM

Actually, depending on the implementation of the the cache and the Linq provider, what's probably happening is that the .Count() call is running through the entire return sequence, leaving the enumerable at the end of the sequence. Then when .LastOrDefault is called, well, it's already at the end, so you get the default value (0, I assume?)

Linq queries don't rewind.

Enrique wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-24-2011 11:41 AM

I know is not an issue but you should avoid using Count() != 0 and start using Any()

Patrick Huizinga wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 01-27-2011 7:58 AM

Chris Tavares,

Indeed Linq queries don't rewind, however they do restart.

In this case the Linq query will be run three times.

at:

cachedEpisodes.Count()

cachedEpisodes.LastOrDefault()

AddRange( cachedEpisodes )

Each time .Where( x => x.EpisodeNumber > lastKnownEpisodeId ) will be evaluated with whatever the then current value of lastKnownEpisodeId. Which gets changed after the second time, so the AddRange() will work on a different result set as the Count() and LastOrDefault().

btw Derik, why are you using LastOrDefault when you already know there's at least one item?

Paul wrote re: Linq rocks, except when it doesn’t. Can anyone spot the bug in this code?
on 03-25-2011 9:27 AM

I sometimes include "query" in the name of the LINQ query variable, to make it obvious that it's a query rather than a result.

About The CodeBetter.Com Blog Network
CodeBetter.Com FAQ

Our Mission

Advertisers should contact Brendan

Subscribe
Google Reader or Homepage

del.icio.us CodeBetter.com Latest Items
Add to My Yahoo!
Subscribe with Bloglines
Subscribe in NewsGator Online
Subscribe with myFeedster
Add to My AOL
Furl CodeBetter.com Latest Items
Subscribe in Rojo

Member Projects
DimeCasts.Net - Derik Whittaker

Friends of Devlicio.us
Red-Gate Tools For SQL and .NET

NDepend

SlickEdit
 
SmartInspect .NET Logging
NGEDIT: ViEmu and Codekana
LiteAccounting.Com
DevExpress
Fixx
NHibernate Profiler
Unfuddle
Balsamiq Mockups
Scrumy
JetBrains - ReSharper
Umbraco
NServiceBus
RavenDb
Web Sequence Diagrams
Ducksboard<-- NEW Friend!

 



Site Copyright © 2007 CodeBetter.Com
Content Copyright Individual Bloggers

 

Community Server (Commercial Edition)