One of the best traits of a well-designed system is composability. Large systems are complex and hierarchical and one of the best ways to fight accidental complexity is to compose a system from smaller components. You write and test each component independently then you glue them together to achieve a higher-level behavior.<\/p>\n
Programming languages usually designed to have “composable” features as well. You should be able to use multiple features together and the entire thing should just work. In C# you can compose different features together, and everything works. Unless it isn’t.<\/p>\n
Recently I stumbled across a piece of C# code that shows the complexity of the language and demonstrates how easy these days to make mistakes by combining different language features. C# language is complex, and it is important to understand the features deep enough to use them correctly.<\/p>\n
Let\u2019s review the following code:<\/p>\n
public static IEnumerable<Task<int>> ParseFile(string path)\r\n{\r\n if (string.IsNullOrEmpty(path)) throw new ArgumentNullException(nameof(path));\r\n\r\n \/\/ OpenWithRetryPolicyAsync uses RetryPolicy to try to open the file more than once.\r\n \/\/ The method throws FileNotFoundException if the file does not exists.\r\n using (var file = OpenWithRetryPolicyAsync(path).Result)\r\n {\r\n using (var reader = new StreamReader(file))\r\n {\r\n \/\/ Let's assume that the first line contains a number of entries.\r\n\r\n \/\/ Using int.Parse for simplicities sake\r\n var numberOfEntries = int.Parse(reader.ReadLine());\r\n\r\n for (int entry = 0; entry < numberOfEntries; entry++)\r\n {\r\n yield return ReadAndParseAsync(reader);\r\n }\r\n }\r\n }\r\n}\r\n\r\nprivate static async Task<int> ReadAndParseAsync(StreamReader reader)\r\n{\r\n string line = await reader.ReadLineAsync();\r\n return int.Parse(line);\r\n}\r\n\r\npublic static IEnumerable<Task<int>> ParseAndLogIfNeeded(string path)\r\n{\r\n try\r\n {\r\n return ParseFile(path);\r\n }\r\n catch (Exception e)\r\n {\r\n Console.WriteLine($\"Failed to parse the file: {e.Message}\");\r\n return Enumerable.Empty<Task<int>>();\r\n }\r\n}<\/pre>\nThe code of OpenWithRetryPolicyAsync<\/code> method is not relevant for our discussion so I moved the code to the end of the blog post.<\/p>\n\n- Exception handling and iterator blocks<\/li>\n<\/ol>\n
Iterator blocks are quite different from regular methods. If a method returns IEnumerable<T><\/code> or IEnumerator<T><\/code> and has yield return<\/code> in it, the compiler completely changes the semantics of the method. The method becomes lazy and it’ll be executed not when the method is called, but rather when the sequence is consumed.<\/p>\nThis behavior may catch you off-guard especially when the result of the iterator block is “transformed” using other lazily evaluated functions, like LINQ operators:<\/p>\n
var s1 = ParseFile(null); \/\/ no exceptions\r\n\r\nvar s2 = s1.Select(t => t.GetAwaiter().GetResult()).Where(r => r == 42); \/\/ no exceptions\r\n\r\nif (s2.Any()) \/\/ throws an exception, potentially in completely different subsystem!\r\n{ }<\/pre>\nThis means that ParseFile<\/code> method never throws when the method is called, and the catch block ParseAndLogIfNeeded<\/code> is effectively unreachable and will never handle any errors.<\/p>\n\n- Observing
ex.Message<\/code> is not enough<\/li>\n<\/ol>\nExceptions are like ogres, they have layers.<\/strong> In many cases, exceptions are nested with absolutely unactionable information in the top-level Message<\/code> property.<\/p>\nLet\u2019s suppose we resolved laziness issue by calling .ToList()<\/code>: try { return ParseFile(path).ToList();}<\/code>.<\/p>\nIf a given path<\/code> does not exist, then OpenWithRetryPolicyAsync<\/code> throws FileNotFoundException<\/code>. But because the method is asynchronous, the actual error will be wrapped in AggregateException<\/code>. And in this case, the log file (the console in this case) will contain a very useful error message: “One or more error occurred”.<\/p>\nAnd AggregateException<\/code> is not the only example: TypeLoadExcpetion<\/code> and TargetInvocationException<\/code> never contain relevant information in Message<\/code>property. In other cases, you need to know a stack trace or an inner exception in order to understand the root cause of an issue.<\/p>\nIn the perfect world, you would never even catch System.Exception<\/code>, but in reality, this suggestion isn’t practical. So if you ended up catching generic exception type without re-throwing it, at least trace the full exception instance. Believe me, you’ll save yourself hours of investigation time in the future.<\/p>\n\n- Prefer calling
GetAwaiter().GetResult<\/code> over .Result<\/code><\/li>\n<\/ol>\nBlocking asynchronous operations is another anti-pattern that should never be used. But similarly to another anti-pattern mentioned above, you have to use it in practice from time to time to avoid viral asynchrony of the code.<\/p>\n
There are 3 ways to synchronously wait for the result of a task-based operation: .Result<\/code>, .Wait<\/code> and GetAwaiter().GetResult()<\/code>. The first two operations will throw AggregateException<\/code> if the task fails and GetAwaiter().GetResult()<\/code> will unwrap and throw the first exception from task’s AggregateException<\/code>.<\/p>\nIn some rare cases, a task is backed by more than one operation and AggregateException<\/code> is what you need. But in the majority tasks are backed by an asynchronous operation that could have only one error, and propagating it directly simplifies exception handling a lot, especially when a caller does not expect to get an AggregateException<\/code> at all.<\/p>\n\n- Using block and the lifetime of asynchronous operations<\/li>\n<\/ol>\n
All the issues we discussed before are important but more or less obvious for experienced developers. The last one is a bit subtler.<\/p>\n
Let\u2019s take a closer look at the lifetime of the file<\/code> variable in ParseFile<\/code>. The compiler generates a state machine and calls a generated “finally block” to close the file when all the items of the sequence are produced. But the problem is that the method “yields” tasks (not values) and if the last task is not synchronous, then the task may read a file when the file is already closed!<\/p>\nHere what may happen if the caller of ParseFile<\/code> calls ToList()<\/code> on the result to eagerly obtain all the elements from the sequence and if the tasks are not completed synchronously:<\/p>\nStep 1: Opening the file \r\nStep 2: Yielding Task0 \r\nStep 3: Yielding Task1 \r\nStep 4: Closing the file \r\nStep 5-6: Tasks 0 and 1 are reading the file and failing with `ObjectDisposeException`<\/pre>\nThe issue may be in your code for years and manifest itself in some weird ways. For instance, the code may work fine on Windows for small files when ReadLineAsync()<\/code>returns synchronously because the data is prefetched, but fail in other cases if ReadLineAsync<\/code> will do an actual IO. (This is actually exactly how we discovered the issue: once we tried to run the code on MacOS we started getting the errors consistently.)<\/p>\nHow to solve the main issue?<\/h4>\n
To be honest, I\u2019m not a big fan of IEnumerable<Task><\/code>. The main issue from my perspective, that the semantics of a method is not clear. Just by looking at a method signature it is hard to tell if the sequence is “hot” or “cold”. If the sequence is “cold” and lazy, then the tasks in the sequence are initiated one by one. If the sequence is “hot” and is backed by a collection, then all the tasks are running already.<\/p>\n“The right” approach depends on your needs. If clients will use all the items of the result all the time, then just use Task<List<T>><\/code>. As the author of a function, you should think about an ease of use and clarity of your functions. Types can be a useful source of information and you should try to express your intent as clearly as possible.<\/p>\nAsynchronous sequences could be useful in some cases. For instance, you may use IEnumerable<Task><\/code> if the sequence contains many elements, the time for getting the next element is high, and clients may need only the first few elements. But in this case, you should explain your intent in comments or, even better, use something like IAsyncEnumerable<T><\/code><\/a>.<\/p>\n