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.

Check the length of the reader before validating the signature ?

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

Are the permissions to the Directory ok?

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.

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

@matt,

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

@Ramon Smits,

that's Ayende's coding style.

I like it.

Where do you close the writer?

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

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!

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.

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.

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

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.

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.

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

// Ryan

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

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

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.

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.

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

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