Challenge: Find the bug previous: NHibernate: Streaming large result sets next: The pitfalls of transparent security The following code contains a bug that would only occur under rare situations, can you figure out what is the bug? Comments 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. 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. 06/29/2010 03:48 PM by David Thibault Check the length of the reader before validating the signature ? 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. 06/29/2010 04:38 PM by Tim Schwallie Are the permissions to the Directory ok? 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. 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. 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). 06/29/2010 08:25 PM by Louis Haußknecht @Ramon Smits, that's Ayende's coding style. I like it. 06/29/2010 09:34 PM by Stephane Where do you close the writer? 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). 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! 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. 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. 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.) 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. 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. 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. 06/30/2010 08:23 AM by Ayende Rahien Mattia, I don't worry about that, I'll get the appropriate error then. 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 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 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. 06/30/2010 10:55 AM by Ayende Rahien Matthew, Good catch, but that still isn't it 06/30/2010 10:55 AM by Ayende Rahien @Everyone, Here is a big hint, think C++ 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 ... 06/30/2010 11:08 AM by Ayende Rahien Gerard, Not it either 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. 06/30/2010 11:33 AM by Ayende Rahien Matthew, Yes, you are right, what is the result? 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. 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. 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. 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.