Ayende @ Rahien

It's a girl

Challenge: Spot 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?

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

Phillip Haydon
09/26/2013 09:05 AM by
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
09/26/2013 09:12 AM by
Ales

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

tobi
09/26/2013 09:16 AM by
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
09/26/2013 09:16 AM by
Ivo

var fileInfo = new FileInfo(file);

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

tobi
09/26/2013 09:17 AM by
tobi

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

thaianhduc
09/26/2013 09:17 AM by
thaianhduc

This call will throw exception: MemoryMapPager("")

Łukasz Wiatrak
09/26/2013 09:18 AM by
Ł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
09/26/2013 09:20 AM by
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
09/26/2013 09:21 AM by
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
09/26/2013 09:21 AM by
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
09/26/2013 09:22 AM by
Gareth Evans

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

Ali
09/26/2013 09:23 AM by
Ali

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

mihasic
09/26/2013 09:25 AM by
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
09/26/2013 09:25 AM by
Humanitis

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

Ade
09/26/2013 09:26 AM by
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
09/26/2013 09:27 AM by
gabon

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

Cheers

Thomas Levesque
09/26/2013 09:28 AM by
Thomas Levesque

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

Sergey Shumov
09/26/2013 09:50 AM by
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
09/26/2013 10:06 AM by
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
09/26/2013 10:45 AM by
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
09/26/2013 11:01 AM by
Richard Dingwall

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

Michael
09/26/2013 12:18 PM by
Michael

The method signature is missing a return type.

Scooletz
09/26/2013 12:26 PM by
Scooletz

@Michael, it's a ctor.

Jacek
09/26/2013 01:08 PM by
Jacek

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

markrendle
09/26/2013 01:10 PM by
markrendle

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

Andreas McDermott
09/26/2013 01:21 PM by
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
09/26/2013 02:54 PM by
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
09/26/2013 07:00 PM by
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
09/26/2013 07:32 PM by
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
09/26/2013 07:32 PM by
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
09/27/2013 12:32 AM by
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
09/27/2013 07:16 AM by
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
09/27/2013 07:20 AM by
macpak

You use file.Length instead of fileInfo.Length.

Raif
09/27/2013 07:20 AM by
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
09/27/2013 07:24 AM by
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
09/27/2013 08:54 AM by
Rob Lyndon

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

Aaron
09/27/2013 03:17 PM by
Aaron

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

Luky Wong
09/27/2013 04:59 PM by
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
10/14/2013 07:23 AM by
Alexandr Nikitin

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

Ayende Rahien
10/14/2013 07:25 AM by
Ayende Rahien

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

Comments have been closed on this topic.