AuthJanitor icon indicating copy to clipboard operation
AuthJanitor copied to clipboard

ViewModelFactory::GetViewModel forces sync over async which could lead to a deadlock

Open drub0y opened this issue 5 years ago • 0 comments

ViewModelFactory::GetViewModel, which has several overloads, is implemented as a synchronous method. However, it makes a call to IDataStore<Resource>::GetOne which is an asynchronous method. It forces this call to be blocking by accessing the .Result property of the Task returned from the call to GetOne. This can lead to deadlocks and other performance issues.

It's easy enough to change GetViewModel to be async, but it will cause ripples through the codebase (as going fully asynchronous usually does).

Proposed steps:

  1. Change GetViewModel overloads to be async - this is actually the easy part. It differs slightly per overload, but it will be easy enough to go make each of them async.
  2. Change all callers to expect GetViewModel calls to be async - this is the harder part, let's dig into why. First, GetViewModel is actually just a backing implementation for a bunch of Func<T, VM> factory methods that get registered into the services collection at start up. I actually like this, it's an elegant way to abstract the caller from how the view models are resolved without the need to introduce your own factory interface (love me some Func<>). Now, what would need to happen to propagate the asynchronous behavior is as follows: a. All of these Func<T, VM> would now need to become Func<T, Task<VM>> instead. b. All dependents who are expecting these to be injected would match that previous signature change c. All dependents need to be updated to await the result of the factory method instead of expecting it synchronously. d. Any dependent methods that were previously synchronous will also have to be updated to be async. e. Any callers of dependent methods of d now need to await those methods and if they themselves were synchronous, now go to step d again for its dependents and rinse and repeat until you get all the way to the top.

So it's a bunch of menial work which will not only ensure that you benefit from the asynchronous nature of the underlying call, but, more importantly, ensure you don't end up in a deadlock situation.

drub0y avatar May 12 '20 23:05 drub0y