Fix async deadlock

[3nth]

Ran into this deadlock while using in .NET 4.7.2 ASP.NET MVC 5 project. Didn't occur in integration testing.

Uninstalled nuget and loaded project directly and used debugger to find the call that was deadlocking. Simple fix once found.

[jzabroski]

@3nth Hi Derek, First, thanks for your patience. I was just getting back from vacation when this PR was opened and it fell behind.

Given that the call to GetProjectItem is virtual and RazorLight doesn't own that dependency, is this really the right fix? We can't gaurantee that the underlying code also calls ConfigureAwait(False);

Which RazorLightProject are you using? Is it one of the default ones? Some people implement their own projects, which can lead to deadlock. EmbeddedRazorProject returns Task.FromResult, so it already guarantees no new synchronization context will be created. So does FileSystemRazorProject

[3nth]

@jzabroski Hi John! Thanks for responding.

We implemented our own database/EF backed project, based on this one: https://github.com/toddams/RazorLight/blob/65484c59fd678b6d1b843aaa5c8eb7726c06cf1d/samples/RazorLight.Samples/EntityFrameworkRazorLightProject.cs

I do see the issue with no guarantees on underlying code using ConfigureAwait(false). I did also update the sample to reflect that as well. Not sure if there is a use case for ConfigureAwait(true) out there, so this could be an issue in the case. I am unsure, as always, when it comes to this topic.

Here's our GetItemAsync

        public override async Task<RazorLightProjectItem> GetItemAsync(string templateKey)
        {
            // We expect id to be an integer, as in this sample we have ints as keys in database.
            // But you can use GUID, as an example and parse it here
            var split = templateKey.Split('_');
            int templateID = int.Parse(split[1]);

            string content;
            using (var db = new EbisuContext())
            {
                if (split[0] == "HTML")
                {
                    content = await db.EmailTemplates
                        .Where(e => e.ID == templateID)
                        .Select(t => t.BodyHtml)
                        .SingleOrDefaultAsync()
                        .ConfigureAwait(false);
                }
                else
                {
                    content = await db.EmailTemplates
                        .Where(e => e.ID == templateID)
                        .Select(t => t.BodyText)
                        .SingleOrDefaultAsync()
                        .ConfigureAwait(false);
                }
            }

            var projectItem = new ProjectItem(templateKey, content);

            return projectItem;
        }

Also, recently learned how to write a test that should force a thread context / deadlock issue (if one exists), so I might be able now write a test here.

[jzabroski]

I guess changing it to ConfigureAwait(false) is not a bad idea, but it would be a major breaking change. Even though the releases are tagged beta, many people depend upon it. I'd rather we make this a configuration variable and for now default it to true and you can change it to false.

Basically, in my eyes, ConfigureAwait(false) could be even worse. Imagine this code is used by some government to send people a tax assessment for their business, and they store templates in a db like yours but their code blocks. Then maybe the email never gets sent and is lost silently. I just always try to humanize decisions before making them. It's a fake user story (I hope). I'd rather have a potential deadlock than , say, a controller context that completes before razorlight does. At least then it hangs the process. In my experience, most bad developers never check the Windows Event Log and when a website is slow, they never think it could be because the process crashed and its respawning the process and any IIS modules that need loading, including the Global Init handler for an ASP.NET Classic app. However, a deadlock is a bit more noticeable than a process crashing. (To each his own. - That's why I think its best way make it configurable.)

[3nth]

Totally understand where you are coming from.

As I understand it (and perhaps I do not), the ConfigureAwait() relates to whether the thread needs to rejoin/interact with the parent thread context or if it can run truly in parallel off any old thread in the thread pool. Mostly, this comes up in UI based programming. In MVC5 there is a managed context which is where the HttpContext lives, so if your async code is going to interact with that, then you would need ConfigureAwait(true) .

I do note that the only other call to GetItemAsync does have ConfigureAwait(false) https://github.com/toddams/RazorLight/blob/badbd178564ba59934368161c7c140fa5906eee1/src/RazorLight/Generation/RazorSourceGenerator.cs#L56

Also, when examining the code path from CompileRenderAsync to GetItemAsync it is ConfigureAwait(false) at this point as well, but not clear whether that a) doesn't matter or b) means we might as well do it here cause it's already being done.

https://github.com/toddams/RazorLight/blob/f3a7041bf10b1242182eb3af9ae7a35226b7a5fa/src/RazorLight/EngineHandler.cs#L129

[3nth]

I do think I have another alternative, which is to wrap my outer call to CompileRenderAsync() with Task.Run(() => CompileRenderAsync()), which I believe would create a fresh SychronizationContext separate from the MVC one that perhaps will not deadlock.

BUT... I went to try that. Switch from my patched version to the NuGet release (2.0.0-beta7) and my deadlock is gone without doing anything else. Just wanted to see it deadlock before I tried anything and it does not.

Or is it? But, could pretty reliably deadlock previously when accessing via MVC app (but not in our integration test).

🤷‍♂️

[jzabroski]

Or is it? But, could pretty reliably deadlock previously when accessing via MVC app (but not in our integration test).

I think the key part is you're using ".NET 4.7.2 ASP.NET MVC 5". That will require a synchronization context.

I'm not trying to confuse you. I think your change makes sense, I just think to avoid immediately breaking anyone with worse behavior (because we don't own the caller we await on), we should make the true/false awaiter setting configurable. This will let existing users deadlock, discover their problem, fix their code, and then configure the await appropriately. It can be documented in the release notes, as opposed to being a breaking change.

Put another way, by making the await configurable, for library users who do not make their ProjectSystem "library code-like" and instead intermix application-like details, we can them do that. They would then be responsible for ensuring GetItemAsync internally manages its own computation in a thread-safe, non-deadlocking way. Ideally, they change their code to be "async all the way".

As an example, somebody might implement a ".NET 4.7.2 ASP.NET MVC 5" RazorLightProjectSystem that accesses the HttpContext global Request object. Perhaps to get a user iprincipal.

I believe I am thinking about this correctly. Please let me know if you disagree.

[3nth]

Yes, that is my understanding. If one wanted to reference HttpContext from GetItemAsync, they would require ConfigureAwait(true) behavior.