Critique this code: The memory mapped file
We got an error in the following code, in production. We are trying hard to make sure that we have good errors, which allows us to troubleshoot things easily.
In this case, the code… wasn’t very helpful about it. Why? Take a look at the code, I’ll explain why below…
public CodecIndexInput(FileInfo file, Func<Stream, Stream> applyCodecs) { try { this.file = file; this.applyCodecs = applyCodecs; fileHandle = Win32NativeFileMethods.CreateFile(file.FullName, Win32NativeFileAccess.GenericRead, Win32NativeFileShare.Read | Win32NativeFileShare.Write | Win32NativeFileShare.Delete, IntPtr.Zero, Win32NativeFileCreationDisposition.OpenExisting, Win32NativeFileAttributes.RandomAccess, IntPtr.Zero); if (fileHandle.IsInvalid) { const int ERROR_FILE_NOT_FOUND = 2; if (Marshal.GetLastWin32Error() == ERROR_FILE_NOT_FOUND) throw new FileNotFoundException(file.FullName); throw new Win32Exception(); } mmf = Win32MemoryMapNativeMethods.CreateFileMapping(fileHandle.DangerousGetHandle(), IntPtr.Zero, Win32MemoryMapNativeMethods.FileMapProtection.PageReadonly, 0, 0, null); if (mmf == IntPtr.Zero) { throw new Win32Exception(); } basePtr = Win32MemoryMapNativeMethods.MapViewOfFileEx(mmf, Win32MemoryMapNativeMethods.NativeFileMapAccessType.Read, 0, 0, UIntPtr.Zero, null); if (basePtr == null) throw new Win32Exception(); stream = applyCodecs(new MmapStream(basePtr, file.Length)); } catch (Exception) { Dispose(false); throw; } }
Did you see it?
This code has multiple locations in which it can throw Win32Exception. The problem with that is that Win32Exception in this mode is pretty much just a code, and we have multiple locations inside this method that can thrown.
When that happens, if we don’t have the PDB files deployed, we have no way of knowing, just from the stack trace (without line numbers), which of the method calls had caused the error. That is going to lead to some confusion after the fact.
We solved this by adding description text for each of the options, including additional information that will let us know what is going on. In particular, we also included not only the operation that failed, but even more important, we included the file that failed.
Comments
That's a very good technique. I find that this is almost never being done. I admit that I learned this way to late.
I find that wrapping exceptions and adding information is often way better than catching, logging and rethrowing. Let the caller decide how to catch and log.
Proper error handling is the last stepping stone in order to become a good developer, and to be able to truly create quality software and products .
Out industry puts way to little emphasis on propper error handlig and too much focus on patternizing and unittestifying the code. But this will not lead to good code, at most it can transform bad code to mediocre code. Unless you have good and complete error handling (and done properly) you will not have good code, the industry needs to learn this.
It is good to have such complex things in the constructor? I always try to avoid doing that.
EQR, You have to do it _somewhere_, and ctor vs. init method doesn't really change things much.
Oren, It may be just my mindset but I do not expect a contructor to fail. Factory method is opposite, it indicates that something unobvious occurs here, it may be dangerous and here be dragons.
EQR, Ctor failure mode is a bit complex, because in .NET, if you have Finalizers, you need to handle it, but that is about it. We already handle this fairly often in our codebase.
It's a good reminder. Know what you throw.
If you're using C#6 this is the entire purpose of exception filters
catch (SomeDependencyException ex) when ( use logging code that has CallerMemberName, CallerLineNumber, etc, return true)
The WHEN fragment executes in the scope of where the error was generated without having the stack frame mutated.
It highly annoys me that almost all articles that talk about exception filters use horribly contrived examples and leave out the most important part. That you can use the Caller attributes for where the exception happened.
The code style is inconsistent.
A really better place to do this job would be a factory which would create/init the arguments and then pass them.
Chris, This will record where the when is called, not the original error location
Comment preview