Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 5,971 | Comments: 44,508

filter by tags archive

ChallengeSpot the bug


Speaking of unfair interview questions, this would be a pretty evil one.

image

Can you see the bug? And how would you fix it?

More posts in "Challenge" series:

  1. (28 Apr 2015) What is the meaning of this change?
  2. (26 Sep 2013) Spot the bug
  3. (27 May 2013) The problem of locking down tasks…
  4. (17 Oct 2011) Minimum number of round trips
  5. (23 Aug 2011) Recent Comments with Future Posts
  6. (02 Aug 2011) Modifying execution approaches
  7. (29 Apr 2011) Stop the leaks
  8. (23 Dec 2010) This code should never hit production
  9. (17 Dec 2010) Your own ThreadLocal
  10. (03 Dec 2010) Querying relative information with RavenDB
  11. (29 Jun 2010) Find the bug
  12. (23 Jun 2010) Dynamically dynamic
  13. (28 Apr 2010) What killed the application?
  14. (19 Mar 2010) What does this code do?
  15. (04 Mar 2010) Robust enumeration over external code
  16. (16 Feb 2010) Premature optimization, and all of that…
  17. (12 Feb 2010) Efficient querying
  18. (10 Feb 2010) Find the resource leak
  19. (21 Oct 2009) Can you spot the bug?
  20. (18 Oct 2009) Why is this wrong?
  21. (17 Oct 2009) Write the check in comment
  22. (15 Sep 2009) NH Prof Exporting Reports
  23. (02 Sep 2009) The lazy loaded inheritance many to one association OR/M conundrum
  24. (01 Sep 2009) Why isn’t select broken?
  25. (06 Aug 2009) Find the bug fixes
  26. (26 May 2009) Find the bug
  27. (14 May 2009) multi threaded test failure
  28. (11 May 2009) The regex that doesn’t match
  29. (24 Mar 2009) probability based selection
  30. (13 Mar 2009) C# Rewriting
  31. (18 Feb 2009) write a self extracting program
  32. (04 Sep 2008) Don't stop with the first DSL abstraction
  33. (02 Aug 2008) What is the problem?
  34. (28 Jul 2008) What does this code do?
  35. (26 Jul 2008) Find the bug fix
  36. (05 Jul 2008) Find the deadlock
  37. (03 Jul 2008) Find the bug
  38. (02 Jul 2008) What is wrong with this code
  39. (05 Jun 2008) why did the tests fail?
  40. (27 May 2008) Striving for better syntax
  41. (13 Apr 2008) calling generics without the generic type
  42. (12 Apr 2008) The directory tree
  43. (24 Mar 2008) Find the version
  44. (21 Jan 2008) Strongly typing weakly typed code
  45. (28 Jun 2007) Windsor Null Object Dependency Facility

Comments

Phillip Haydon

Hmmm, taking a while guess, I assume if you have a 0 byte file then it would attempt to create it and would throw since it already exists?

Ales

file is string which contains filename. How can you decide from length of file name number of pages ? A.

tobi

Creating the file is not exception-safe, there is a race between creating it and opening it (not sure whether that one matters). I'd make this into a single new FileStream call with different flags as input.

Ivo
Ivo

var fileInfo = new FileInfo(file);

this line will throw an expection if the file text is empty

tobi

Also, creating over a zero-byte file will do that without FileShare.Read.

thaianhduc

This call will throw exception: MemoryMapPager("")

Łukasz Wiatrak

Wild guess: theoretically, there is a tiny interval between fileInfo.Exists and fileInfo.Create statements, some other process/thread could create and open this file during this interval (so e.g. exception about already opened file could be thrown). It reminds me this answer on SO (but in python): http://stackoverflow.com/a/85237/543205

Stefan Mindegaard Lyager

Considering the following line:

_allocatedPages = file.Length / PageSize;

file.Length is the length of the path to the file, not the length of the file itself.

Cristi Ursachi

problem I see is that if Open fails then you have allocatedPages set an you should not(since is class field), also you don't need that much code, to avoid it I would do it like this:

public MemoryMapPager(string file,FlushMode flushMode.Full) { fileStream = new FileStream(filePath, FileMode.OpenOrCreate, FileAccess.ReadWrite);

_allocatedPages=fileStream.Length/PageSize;

}

Rob
Rob

Wasn't the second answer (@Ales) correct? Condition should say:

if (fileInfo.Exists == false || fileInfo.Length == 0)

I read FileStream and FileInfo code first looking for something tricky and then noticed this :) Unit tests would catch it - compiler had no chance.

Gareth Evans

To expand on @Ales's answer you should used fileInfo.Length not file.Length.

Ali
Ali

In case file exists, fileInfo does not get closed? Soooo, did I get the job?

mihasic

Don't know the real context, but assume it should be something like this:

if (fileInfo.Exists == false) { fileInfo.Create().Close(); } _allocatedPages = fileInfo.Length / PageSize;

Humanitis

_allocatedPages=fileStream.Length/PageSize + (fileStream.Length%PageSize>0?1:0);

Ade
Ade

Yep, both instances of file.Length should be fileInfo.Length. You might also have an issue if the directory doesn't exist, maybe add fileInfo.Directory.Create() before creating the file.

gabon

This line looks strange _allocatedPages = (file.Length / PageSize); it should be +1

Cheers

Thomas Levesque

That's why I avoid calling "file" a variable that contains a path...

Sergey Shumov

The code should be like this: _allocatedPages = Math.Ceiling(fileInfo.Length / (double) PageSize);

So if the file length is 20 and the size of page is 17 then the number of allocated pages should be 2.

chris

would it be evil? You'd get to see how the candidate approached the problem, Ales spotted the major "typo that compiles" problem, but an emergent discussion about race conditions, divide by zero etc etc would make for a nice ice breaker.

hangy

Obviously, file might be null or empty (throw ArgumentException), and checking for the length of the filename is wrong. However, do we know that PageSize has been initialized with a non-zero value (division by zero)? Additionally, why check if the file exists, possibly create (and close) and then open it again? One could arguably just use FileMode.OpenOrCreate and use _fileStream.Length to calculate _allocatedPages. That would get rid of race conditions and some code.

Richard Dingwall

"string file" -- poor variable naming caused this bug.

Michael

The method signature is missing a return type.

Scooletz

@Michael, it's a ctor.

Jacek

So I'm not sure if it's a good idea to create/open file in ctor.

markrendle

The obvious bug aside, isn't it a bad idea to do IO things like creating and opening files in constructors?

Andreas McDermott

file.Length is used in a couple of places where I'm assuming you are not interested in the length of the file path. :)

allan nienhuis

Speaking of interviews - watching someone review unfamiliar code like this is orders of magnitude more useful than FizzBuzz, and way less stressful for the applicant, IMO.

Jason Meckley

fileinfo.Open will throw if the file did not exist in the first place. I just ran into this problem. it's the difference between System.IO.File and System.IO.FileInfo

hangy

So I'm not sure if it's a good idea to create/open file in ctor.

The obvious bug aside, isn't it a bad idea to do IO things like creating and opening files in constructors?

I think that should be okay if that is the documented and expected behavious, not an unexpected side-effect.

fileinfo.Open will throw if the file did not exist in the first place. I just ran into this problem. it's the difference between System.IO.File and System.IO.FileInfo

Well, that should depend on the FileMode parameter since all that Open does is construct a new FileStream.

Alexandr Nikitin

Ups, sorry, can't paste gists. Here's code:

public MemoryMapPager(string fileName, FlushMode flushMode = FlushMode.Full) { _flushMode = flushMode;

var fileInfo = new FileInfo(fileName);
var fileLength = fileInfo.Length;
var isFileEmpty = fileLength == 0 || fileInfo.Exists == false;

_fileStream = fileInfo.Open(FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.Read);

_allocatedPages = isFileEmpty ? 0 : (long) Math.Ceiling((float) fileLength/PageSize);

}

Oğuzhan Eren
  • Why re-create file on if "file.Length==0" case? _allocatedPages = (file.Length / PageSize) need to check PageSize must greather than zero and MemoryMapPager need to implement IDisposable interface for disposing FileStream
Raif

Some AV software scan on file create after the file handle is closed, on an enough heavily-loaded machine, you will get an UnauthorizedAccessException once in a blue moon in the fileInfo.Open line.

Fixing it is as simple as touching the file only once (as well as the typos).

macpak

You use file.Length instead of fileInfo.Length.

Raif

Also, _fileStream isn't being initialized in all cases. This means in case the file doesn't exist or (after fixing the name of the file variable and changing the length check to that of the FileInfo), you may get NullReferenceException's trying to use the instance you just created.

If the intention is to not allow a MemoryMapPager to be constructed, an InvalidOperationException or similar should be thrown to catch the problem earlier on.

Raif

The class name is also misleading, the name "Pager" implies that this is concerned only with the reading and paging of things. I'm not an expert on memory mapped views but it seems weird to me that it should care about FlushMode.

If the class will be used like a regular memory mapped view (sliding window over a memory space with read/write access), it should be named better. Perhaps, MemoryMappedView ? or if it is dealing with memory view the size of memory pages or disk-pages, then MemoryMapPage is better (without the "r").

Rob Lyndon

Quite a good argument in favour of a TDD approach, this one.

Aaron

I think the important code defect is that you've named the first parameter "file" and not something like "fileName".

Luky Wong

Change file.Length to fileInfo.Length, the length of a file's is different from the length of a string named file.

Alexandr Nikitin

Oren, the bugs are pretty obvious, but anyway we're waiting for your comments :)

Ayende Rahien

Alexandr, You found them. The actual bug causing this post was the file.Length issue.

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. Paying the rent online - about one day from now

There are posts all the way to Aug 03, 2015

RECENT SERIES

  1. Production postmortem (5):
    29 Jul 2015 - The evil licensing code
  2. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
  3. API Design (7):
    20 Jul 2015 - We’ll let the users sort it out
  4. What is new in RavenDB 3.5 (3):
    15 Jul 2015 - Exploring data in the dark
  5. The RavenDB Comic Strip (3):
    28 May 2015 - Part III – High availability & sleeping soundly
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats