http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660System.IO.PathTooLongException, too. (Folder name is valid, file name is valid, combination is big kablooie). http://ayende.com/3401/challenge-find-the-bug#comment28http://ayende.com/3401/challenge-find-the-bug#comment28Sun, 06 Jul 2008 11:03:12 GMTJames, That stuff always happen when using somebody library (Checking for something else that will be check again) But it's good programming practice anyway especially when using somebody else lib, unless the name is CreateIfNotExist. Oren, ahh... the beauty of relative path :) I see the bug now. I've encounter this bug before I end up caching the first current directory and lock the user to that path your requirement might be different. http://ayende.com/3401/challenge-find-the-bug#comment27http://ayende.com/3401/challenge-find-the-bug#comment27Fri, 04 Jul 2008 16:02:53 GMTIf changing the current dir was an issue, wwfDev is right regarding Path.Combine. From msdn doc: "Return Value Type: System..::.String A string containing the combined paths. If one of the specified paths is a zero-length string, this method returns the other path. *If path2 contains an absolute path, this method returns path2.*" I'm glad I thought of most of the potential issues mentioned here before reading the comments (multi-thread, relative path, path.combine, combined path too long). Made me feel like I am a better dev than I really am. :) http://ayende.com/3401/challenge-find-the-bug#comment26http://ayende.com/3401/challenge-find-the-bug#comment26Fri, 04 Jul 2008 13:38:17 GMT@Ngoc Van Nope, Directory.Create will correctly handle that situation, creating all needed subdirectories. Also, note that the Directory.Exist call is unnecessary as Directory.Create handles that. http://ayende.com/3401/challenge-find-the-bug#comment25http://ayende.com/3401/challenge-find-the-bug#comment25Fri, 04 Jul 2008 12:59:58 GMTYeah... that is an issue. The bug I was looking for was changing the current directory, those breaking the implicit assumption in the code http://ayende.com/3401/challenge-find-the-bug#comment24http://ayende.com/3401/challenge-find-the-bug#comment24Fri, 04 Jul 2008 12:12:31 GMTI got it: Even within a single thread, multiple calls to get the same logger can be made. Thus, more than one StreamWriter must be opened to the same file, which will fail. example: public class Foo { public static void Main() { StreamWriter logger = new Log(temp).GetLogger("default.log"); //... new Bar.Work(); } } public class Bar { public void Work() { // cannot open two StreamWriters to the same file StreamWriter logger = new Log(temp).GetLogger("default.log"); } } http://ayende.com/3401/challenge-find-the-bug#comment23http://ayende.com/3401/challenge-find-the-bug#comment23Fri, 04 Jul 2008 12:06:17 GMTYes, you can. http://ayende.com/3401/challenge-find-the-bug#comment22http://ayende.com/3401/challenge-find-the-bug#comment22Fri, 04 Jul 2008 11:31:25 GMTCan I assume that the path argument in the constructor is always non-null, not empty and does not contain illegal characters (? * etc.)? http://ayende.com/3401/challenge-find-the-bug#comment21http://ayende.com/3401/challenge-find-the-bug#comment21Fri, 04 Jul 2008 11:09:25 GMTI just checked Path.Combine again. Can you explain what you are objecting to? http://ayende.com/3401/challenge-find-the-bug#comment20http://ayende.com/3401/challenge-find-the-bug#comment20Fri, 04 Jul 2008 06:47:23 GMTAnyhow, I would stay away from Path.Combine when using input provided by some user/caller (i.e. you are not in control of the contents submitted to .Combine). Take a look at the description of Path.Combine and it will scare you - talk about random results... :-) http://ayende.com/3401/challenge-find-the-bug#comment19http://ayende.com/3401/challenge-find-the-bug#comment19Fri, 04 Jul 2008 06:42:44 GMTThe problem with Directory.Create: Directory.Exists("C:\\NotExists") = false Directory.Exists("C:\\NotExists\OfcourseNotExists") = false Directory.Create("C:\\NotExists\OfcourseNotExists") = exception http://ayende.com/3401/challenge-find-the-bug#comment18http://ayende.com/3401/challenge-find-the-bug#comment18Fri, 04 Jul 2008 02:28:00 GMTOren, you also have a potential long path issue. If the combination of path and name overflow the maximum length of a path in Windows (I believe in the .NET APIs, the default max length is 255 chars), then the File.Create will fail (I'm thinking the File.Append would too). I used to run into this all the time when writing extraction software that might extract an archive that contains an archive that contains an archive. It wasn't "uncommon" to run into the maximum length issue in Windows. http://ayende.com/3401/challenge-find-the-bug#comment17http://ayende.com/3401/challenge-find-the-bug#comment17Fri, 04 Jul 2008 01:48:31 GMTMark, That was a bug when I updated the code. Shane is correct, AppendText was supposed to be there (and is now) http://ayende.com/3401/challenge-find-the-bug#comment16http://ayende.com/3401/challenge-find-the-bug#comment16Thu, 03 Jul 2008 22:58:04 GMTTetsuo , Precisely! And a something that is not obvious (at least to me) until you give it some thought. This is a class of problem that can be classed as "assuming fixed context" http://ayende.com/3401/challenge-find-the-bug#comment15http://ayende.com/3401/challenge-find-the-bug#comment15Thu, 03 Jul 2008 22:54:54 GMTmendicant, Interesting, I haven't thought of that. That is not what I was thinking about, however http://ayende.com/3401/challenge-find-the-bug#comment14http://ayende.com/3401/challenge-find-the-bug#comment14Thu, 03 Jul 2008 22:53:31 GMTHmm... Does this even compile? One return a StreamWriter one return a StreamReader... http://ayende.com/3401/challenge-find-the-bug#comment13http://ayende.com/3401/challenge-find-the-bug#comment13Thu, 03 Jul 2008 22:44:32 GMTMarkk has definitely found a bug, but I don't think it is the original bug Oren intended for us to find. That part of the code was not in the original code written for us to solve ( I think in Oren's haste to modify the original text to remove the possibility of overwriting existing logs he mistakenly chose OpenText instead of the proper AppendText) http://ayende.com/3401/challenge-find-the-bug#comment12http://ayende.com/3401/challenge-find-the-bug#comment12Thu, 03 Jul 2008 22:43:16 GMTding ding ding.... I think Markk got it. http://ayende.com/3401/challenge-find-the-bug#comment11http://ayende.com/3401/challenge-find-the-bug#comment11Thu, 03 Jul 2008 22:38:43 GMTHere's a wild stab in the dark: File.CreateText returns a StreamWriter which implements TextWriter. File.OpenText returns a StreamReader which implements TextReader. ergo when you OpenText, you do so to read, not to write?! So if the file doesnt exist, then booom, exception time. http://ayende.com/3401/challenge-find-the-bug#comment10http://ayende.com/3401/challenge-find-the-bug#comment10Thu, 03 Jul 2008 22:34:17 GMTmeant to type --> which is (most likely) not going to exist. http://ayende.com/3401/challenge-find-the-bug#comment9http://ayende.com/3401/challenge-find-the-bug#comment9Thu, 03 Jul 2008 21:52:59 GMTYou could pass in a relative path that does not exist to the constructor - say "MyLogs\ThisProgram". The constructor would then create the new directory based on the current directory. After this, the path is stored by the class in the relative path form. SO... if the current directory changes and then GetLogger is called, Path.Combine is going to use the new current directory + the relative directory which is (not likely) not going to exist. http://ayende.com/3401/challenge-find-the-bug#comment8http://ayende.com/3401/challenge-find-the-bug#comment8Thu, 03 Jul 2008 21:50:09 GMTok, assuming a single thread model (single app?), my first assertion is not valid. I stand by my second assertion that subsequent calls will delete the contents of previously written logs. I checked the documentation this time :) "If the file does exist, its contents are overwritten." So, if (assuming that) the intention was to append a log entry to existing contents, then there is a subtle bug here as previously written entries will be overwritten since the call to "CreateText" recreates the file as new if it already exists. http://ayende.com/3401/challenge-find-the-bug#comment7http://ayende.com/3401/challenge-find-the-bug#comment7Thu, 03 Jul 2008 21:46:16 GMTIf path refers to a file, ex c:\foo where foo is a text file, Directory.Exists("C:\\foo") will return false, but then Directory.CreateDirectory("c:\\foo") will throw an exception. http://ayende.com/3401/challenge-find-the-bug#comment6http://ayende.com/3401/challenge-find-the-bug#comment6Thu, 03 Jul 2008 21:46:11 GMTYou need to call Log(string path) first to set the path field before you can get your logger, else it should throw an exception. http://ayende.com/3401/challenge-find-the-bug#comment5http://ayende.com/3401/challenge-find-the-bug#comment5Thu, 03 Jul 2008 21:39:27 GMTShane, Assume single threaded access and proper use of using. I updated the code to include open/ create as needed, because that wasn't the point of the bug http://ayende.com/3401/challenge-find-the-bug#comment4http://ayende.com/3401/challenge-find-the-bug#comment4Thu, 03 Jul 2008 21:36:00 GMTThreading issue if 2 threads create a new Log with the same path? Directory.Exists() could return false for both, then they will both try to create it, and the second one will get an exception. http://ayende.com/3401/challenge-find-the-bug#comment3http://ayende.com/3401/challenge-find-the-bug#comment3Thu, 03 Jul 2008 21:09:41 GMTOh, and since we're "creating" a file each time, any subsequent calls to the GetLogger will completely obliterate any previously written logs to the same file name (unless the CreateText call will not actually create the file if it already exists and will instead return a stream to append, but I would have to check the documentation to be sure on that) http://ayende.com/3401/challenge-find-the-bug#comment2http://ayende.com/3401/challenge-find-the-bug#comment2Thu, 03 Jul 2008 21:09:19 GMTMost obvious potential for error is if there is a second call made to log an entry to the same file as in the first call, and the caller of the first call is still writing to the log, an error will be thrown in the second call since the file is still locked as a result of the first call. http://ayende.com/3401/challenge-find-the-bug#comment1http://ayende.com/3401/challenge-find-the-bug#comment1Thu, 03 Jul 2008 21:06:21 GMT