I encountered an issue that I hadn’t seen before, which was not easily apparent and think the cause is worth sharing. There was an error occurring in the browser due to missing data, but working backwards to determine why ended up with it looking like it was missing from results returned by the database. This didn’t seem right and sure enough reproducing the query execution returned the expected data. It took a while, but I eventually realised the cause.
We had a method like the following responsible for executing some SQL and returning the results.
This is simplified from the real method, but the important things to note are
- the anonymous method passed into ExecuteReaderAsync is asynchronous
- and the anonymous method does not return a value, but instead has the side effect of populating the results List.
The problem was that there was no implementation of ExecuteReaderAsync that actually handled an async method. These were the two overloads available
In our ExecuteQuery method we appeared to be targeting the first overload (the one taking an Action) because the anonymous method had no return and we were not assigning the output from the ExecuteReaderAsync method to a variable. However the ExecuteReaderAsync method was getting resolved by the second overload (that takes a Func), because since the anonymous method is async it actually has a return type of Task and was a Func not an Action. The implementation for the second overload overload looks like
Did you notice that the Task returned by the async anonymous method was not awaited and is returned directly? This can result in the command variable being disposed before readAction completes, as well any code in _pool.ExecuteAsync that follows the anonymous method execution also running before readAction completes.
Also further back in our ExecuteQuery method, we were not expecting a Task to be returned from our ExecuteReaderAsync call and never awaited it. This can cause the results List to be returned before all the data has been read from the query.
This is what was happening in the error I was investigating. The missing data was at the end of the query and the results were being returned before the data was added.
This was a tricky error. The code compiled and also passed code review (threading is hard, even with async await). Ultimately we decided to only have one overload of ExecuteReaderAsync.
Firstly, the fact that the anonymous method in ExecuteQuery worked by a side effect was a bad design. While it was tempting to think of the code in the anonymous method as being part of ExecuteQuery it should really be thought of as its own method. By requiring ExecuteReaderAsync to take a Func that returns a result we are trying to encourage anonymous methods to be designed without using side effects. It is still possible however, and that is something code reviews will have to be careful to check.
Secondly the Func should be returning a type of Task, which can then be awaited inside ExecuteReaderAsync. This was justifiable because everywhere that uses a DbDataReader should be reading asynchronously anyway.