Ayende @ Rahien

Refunds available at head office

Challenge: Find the bug

The following code contains a bug that would only occur under rare situations, can you figure out what is the bug?

image

Comments

Jason Meckley
06/29/2010 03:15 PM by
Jason Meckley

isNewDatabase = write.Length == 0;

If (isNewDatabase)

CreateFromScratch();

There doesn't appear to be locking. In rare cases you may have 2 threads call CreateFromScratch() at that same time.

Ayende Rahien
06/29/2010 03:19 PM by
Ayende Rahien

Jason,

No, I won't. the writer FileStream ensures that only one thread access this code.

David Thibault
06/29/2010 03:48 PM by
David Thibault

Check the length of the reader before validating the signature ?

matt
06/29/2010 04:02 PM by
matt

race condition in the 1st two lines between checking for the existence of the directory and creating it.

Tim Schwallie
06/29/2010 04:38 PM by
Tim Schwallie

Are the permissions to the Directory ok?

Ramon Smits
06/29/2010 08:02 PM by
Ramon Smits

Why the weird ==false check and not using if(! construct? THen again, AFAIK the Directory.Create will do nothing when it already exists. So the check is useless.

Andy K
06/29/2010 08:06 PM by
Andy K

Not knowing what CreateFromScratch does I would guess that you don't want to TryReadingFromExistingFile when isNewDatabase.

James Curran
06/29/2010 08:17 PM by
James Curran

@matt,

nope. The .Net CreateDirectory silently NOPs if the directory is already present (the if() is superfluous).

Louis Haußknecht
06/29/2010 08:25 PM by
Louis Haußknecht

@Ramon Smits,

that's Ayende's coding style.

I like it.

Stephane
06/29/2010 09:34 PM by
Stephane

Where do you close the writer?

Chris C
06/29/2010 10:29 PM by
Chris C

If the file is deleted whilst the writer is open then OpenReader will fail (assuming open reader is creating a new file stream).

Rik Hemsley
06/29/2010 10:34 PM by
Rik Hemsley

I've parsed this a few times and can't see the bug. I write similar code often so I'll be checking back to see what you reveal as the answer in case I've fallen into the same trap!

Steve Py
06/29/2010 10:56 PM by
Steve Py

I suspect the writer is module level and left open so that data can be streamed to file. It has read-level sharing.

the check on writer.Length == 0 doesn't sit right with me. It looks like you want to check to see if the file was newly created then call the CreateFromScratch().

I'd have elected for:

storageFile = Path.Combine...

isNewDatabase = File.Exists(storageFile);

writer = ...

if (isNewDatabase)

CreateFromScratch()

But that's just semantics, I don't think that's the cause of any bug...

Two instances of the same application implementing Raven DB could be a problem if started up around the same time.

Simon Labrecque
06/30/2010 12:50 AM by
Simon Labrecque

Not sure if you'consider it a bug since you kinda expect an exception anyway, but

if (new Guid(binaryReader.ReadBytes(16)) != HeaderSignatureGuid)

will generate an ArgumentException when the file exists but is not at least 16 bytes long.

Martin
06/30/2010 05:48 AM by
Martin

If the path parameter is a drive letter (eg "C:") then Path.Combine won't create a valid path. You'll get "C:storage.raven" instead of "C:\storage.raven".

(unless of course you're appending the slash in the caller.)

Geert Baeyaert
06/30/2010 06:14 AM by
Geert Baeyaert

The race condition is not between the first two lines, but between creating the directory and creating the file.

Directory.CreateDirectory throws an IOException when the directory is read-only or not empty.

Ayende Rahien
06/30/2010 06:47 AM by
Ayende Rahien

David,

No, that is okay, we assume that len = 0 is always okay.

Matt,

Ignore race conditions, as other have pointed out, it will work, but the problem isn't with a race condition.

Tim,

No, bad permissions would cause it to crash with an expected error.

Andy,

Actually, that is okay, I always want to read from the file. CreateFromScratch just set things up for me.

Stephane,

The writer is close is the Dispose() method.

Chris,

Ignore any race conditions, they aren't required to show the bug.

Steve,

That is not a problem, only one instance of RavenDB can own a file at a given time.

Simon,

Thanks, that is expected and not what I meant.

Mattia
06/30/2010 07:52 AM by
Mattia

You don't check if you have enough space on disk before creating file. This may result in a IOException if the disk is full.

Ayende Rahien
06/30/2010 08:23 AM by
Ayende Rahien

Mattia,

I don't worry about that, I'll get the appropriate error then.

Ryan Heath
06/30/2010 10:05 AM by
Ryan Heath

Why is FileShare.Delete allowed? Writing to a file that is allowed to be deleted, that looks like a bug to me ...

// Ryan

Ayende Rahien
06/30/2010 10:12 AM by
Ayende Rahien

Ryan,

That means that you can delete the file, which is important for some scenarios (cleanup, mostly).

That is not a bug

Matthew Wills
06/30/2010 10:51 AM by
Matthew Wills

If path wasn't an existing directory but was an existing FILE then the CreateDirectory would throw IOException.

Ayende Rahien
06/30/2010 10:55 AM by
Ayende Rahien

Matthew,

Good catch, but that still isn't it

Ayende Rahien
06/30/2010 10:55 AM by
Ayende Rahien

@Everyone,

Here is a big hint, think C++

Gerard Kappen
06/30/2010 10:57 AM by
Gerard Kappen

I'm not sure if you're aiming for some sort of backward compatibility in your storage file, but if so, I would assume you'd rather compare the version as

if (version > Version)

throw ...

Matthew Wills
06/30/2010 11:12 AM by
Matthew Wills

You are throwing an exception from a constructor. So I suppose you could have a partially constructed object - not sure what impact this would have on the disposal of the writer, for instance.

Ayende Rahien
06/30/2010 11:33 AM by
Ayende Rahien

Matthew,

Yes, you are right, what is the result?

Matthew Wills
06/30/2010 11:38 AM by
Matthew Wills

Well, assuming you are using a using block around your use of TransactionStorage, I suspect the writer will stay alive and thus lock the DB. I suspect GC would get it eventually – I haven't tested and thus don’t know for sure.

You likely should change your Dispose method to handle partially constructed objects (ie don’t assume everything is initialized correctly) and then call the Dispose from a catch block in your constructor.

Richard Dingwall
06/30/2010 11:46 AM by
Richard Dingwall

Doing file IO in a ctor is a shooting offense. Wouldn't be surprised if that is a factor.

Ayende Rahien
06/30/2010 04:02 PM by
Ayende Rahien

Matthew,

Ding, ding ding!

You got it.

The dispose is actually never called, because we aren't completing the ctor.

Ori Peleg
07/11/2010 10:48 AM by
Ori Peleg

Awesome, especially for the "Think C++" hint. I wonder why we consider exception-safety differently when we're not in C++.

Comments have been closed on this topic.