http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660Awesome, especially for the "Think C++" hint. I wonder why we consider exception-safety differently when we're not in C++. http://ayende.com/4549/challenge-find-the-bug#comment32http://ayende.com/4549/challenge-find-the-bug#comment32Sun, 11 Jul 2010 10:48:46 GMTMatthew, Ding, ding ding! You got it. The dispose is actually never called, because we aren't completing the ctor. http://ayende.com/4549/challenge-find-the-bug#comment31http://ayende.com/4549/challenge-find-the-bug#comment31Wed, 30 Jun 2010 16:02:57 GMTDoing file IO in a ctor is a shooting offense. Wouldn't be surprised if that is a factor. http://ayende.com/4549/challenge-find-the-bug#comment30http://ayende.com/4549/challenge-find-the-bug#comment30Wed, 30 Jun 2010 11:46:57 GMTWell, 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. http://ayende.com/4549/challenge-find-the-bug#comment29http://ayende.com/4549/challenge-find-the-bug#comment29Wed, 30 Jun 2010 11:38:41 GMTMatthew, Yes, you are right, what is the result? http://ayende.com/4549/challenge-find-the-bug#comment28http://ayende.com/4549/challenge-find-the-bug#comment28Wed, 30 Jun 2010 11:33:38 GMTYou 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. http://ayende.com/4549/challenge-find-the-bug#comment27http://ayende.com/4549/challenge-find-the-bug#comment27Wed, 30 Jun 2010 11:12:27 GMTGerard, Not it either http://ayende.com/4549/challenge-find-the-bug#comment26http://ayende.com/4549/challenge-find-the-bug#comment26Wed, 30 Jun 2010 11:08:02 GMTI'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 ... http://ayende.com/4549/challenge-find-the-bug#comment25http://ayende.com/4549/challenge-find-the-bug#comment25Wed, 30 Jun 2010 10:57:17 GMT@Everyone, Here is a big hint, think C++ http://ayende.com/4549/challenge-find-the-bug#comment24http://ayende.com/4549/challenge-find-the-bug#comment24Wed, 30 Jun 2010 10:55:20 GMTMatthew, Good catch, but that still isn't it http://ayende.com/4549/challenge-find-the-bug#comment23http://ayende.com/4549/challenge-find-the-bug#comment23Wed, 30 Jun 2010 10:55:00 GMTIf path wasn't an existing directory but was an existing FILE then the CreateDirectory would throw IOException. http://ayende.com/4549/challenge-find-the-bug#comment22http://ayende.com/4549/challenge-find-the-bug#comment22Wed, 30 Jun 2010 10:51:53 GMTRyan, That means that you can delete the file, which is important for some scenarios (cleanup, mostly). That is not a bug http://ayende.com/4549/challenge-find-the-bug#comment21http://ayende.com/4549/challenge-find-the-bug#comment21Wed, 30 Jun 2010 10:12:30 GMTWhy is FileShare.Delete allowed? Writing to a file that is allowed to be deleted, that looks like a bug to me ... // Ryan http://ayende.com/4549/challenge-find-the-bug#comment20http://ayende.com/4549/challenge-find-the-bug#comment20Wed, 30 Jun 2010 10:05:52 GMTMattia, I don't worry about that, I'll get the appropriate error then. http://ayende.com/4549/challenge-find-the-bug#comment19http://ayende.com/4549/challenge-find-the-bug#comment19Wed, 30 Jun 2010 08:23:17 GMTYou don't check if you have enough space on disk before creating file. This may result in a IOException if the disk is full. http://ayende.com/4549/challenge-find-the-bug#comment18http://ayende.com/4549/challenge-find-the-bug#comment18Wed, 30 Jun 2010 07:52:40 GMTDavid, 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. http://ayende.com/4549/challenge-find-the-bug#comment17http://ayende.com/4549/challenge-find-the-bug#comment17Wed, 30 Jun 2010 06:47:07 GMTThe 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. http://ayende.com/4549/challenge-find-the-bug#comment16http://ayende.com/4549/challenge-find-the-bug#comment16Wed, 30 Jun 2010 06:14:31 GMTIf 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.) http://ayende.com/4549/challenge-find-the-bug#comment15http://ayende.com/4549/challenge-find-the-bug#comment15Wed, 30 Jun 2010 05:48:46 GMTNot 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. http://ayende.com/4549/challenge-find-the-bug#comment14http://ayende.com/4549/challenge-find-the-bug#comment14Wed, 30 Jun 2010 00:50:54 GMTI 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. http://ayende.com/4549/challenge-find-the-bug#comment13http://ayende.com/4549/challenge-find-the-bug#comment13Tue, 29 Jun 2010 22:56:56 GMTI'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! http://ayende.com/4549/challenge-find-the-bug#comment12http://ayende.com/4549/challenge-find-the-bug#comment12Tue, 29 Jun 2010 22:34:26 GMTIf the file is deleted whilst the writer is open then OpenReader will fail (assuming open reader is creating a new file stream). http://ayende.com/4549/challenge-find-the-bug#comment11http://ayende.com/4549/challenge-find-the-bug#comment11Tue, 29 Jun 2010 22:29:46 GMTWhere do you close the writer? http://ayende.com/4549/challenge-find-the-bug#comment10http://ayende.com/4549/challenge-find-the-bug#comment10Tue, 29 Jun 2010 21:34:18 GMT@Ramon Smits, that's Ayende's coding style. I like it. http://ayende.com/4549/challenge-find-the-bug#comment9http://ayende.com/4549/challenge-find-the-bug#comment9Tue, 29 Jun 2010 20:25:12 GMT@matt, nope. The .Net CreateDirectory silently NOPs if the directory is already present (the if() is superfluous). http://ayende.com/4549/challenge-find-the-bug#comment8http://ayende.com/4549/challenge-find-the-bug#comment8Tue, 29 Jun 2010 20:17:18 GMTNot knowing what CreateFromScratch does I would guess that you don't want to TryReadingFromExistingFile when isNewDatabase. http://ayende.com/4549/challenge-find-the-bug#comment7http://ayende.com/4549/challenge-find-the-bug#comment7Tue, 29 Jun 2010 20:06:18 GMTWhy 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. http://ayende.com/4549/challenge-find-the-bug#comment6http://ayende.com/4549/challenge-find-the-bug#comment6Tue, 29 Jun 2010 20:02:56 GMTAre the permissions to the Directory ok? http://ayende.com/4549/challenge-find-the-bug#comment5http://ayende.com/4549/challenge-find-the-bug#comment5Tue, 29 Jun 2010 16:38:44 GMTrace condition in the 1st two lines between checking for the existence of the directory and creating it. http://ayende.com/4549/challenge-find-the-bug#comment4http://ayende.com/4549/challenge-find-the-bug#comment4Tue, 29 Jun 2010 16:02:55 GMTCheck the length of the reader before validating the signature ? http://ayende.com/4549/challenge-find-the-bug#comment3http://ayende.com/4549/challenge-find-the-bug#comment3Tue, 29 Jun 2010 15:48:20 GMT