Re-write InjectLiveReloadScriptAsync using async/await

[atifaziz]

This PR re-writes WebsocketScriptInjectionHelper.InjectLiveReloadScriptAsync using async/await instead of the more difficult to follow chain of task continuations.

[RickStrahl]

Thanks Aatif,

Normally I'd agree, but I think in this case there's no need to add the overhead of the extra async await state machine. This code is so simple that I don't think there's anything gained here by adding async/await and adding another method for isolating out the Span.

[atifaziz]

This code is so simple

Yeah, there's not much happening in there but non-linear code is generally never simpler. I had to read through it very carefully and reason about it and the cognitive tax was high for what should have been a simple function.

I think this could be subjective so let's move on.

…in this case there's no need to add the overhead of the extra async await state machine.

Actually, it's quite the opposite. ContinueWith will always allocate (the closure) whereas the async-await state machinery won't be boxed unless the operation is going to complete asynchronous. With async-await, you'll get the optimisation for free without needing to worry about how the write will occur.

See also the following tweet by @davidfowl:

You can't avoid all allocations even when all the options are defaulted. ContinueWith needs to capture the execution context. That's going to mean at least an object that has both your callback and options.

where he then goes on to add:

In the async await case, the state machine starts off as a struct and once you go async it is then boxed into an object, that boxing allocation is basically reused for everything

that I don't think there's anything gained here by adding async/await

Now let's talk about correctness, which is much harder to get right with ContinueWith, and I guess I should have pointed this out earlier. You see, there's actually a serious bug lurking in there because you neither inspect tb in the callbacks, nor do you use any TaskContinuationOptions (which are hard to get right too and have their own set of issues). This means that the continuations will run even in the face of the WriteAsync tasks failing. It's as if you had a try-catch where the error is completely suppressed. If we ever add a CancellationToken to the mix then the complexity of getting it right with ContinueWith gets compounded. So this isn't just a re-write to make things pretty, ✨ it's to make the code more efficient, correct, sequential to read and debug and consequently simpler to reason about. There's no element of surprise left.

…adding another method for isolating out the Span.

This is just a bonus. 😄 You get a stand-alone, cohesive and reusable extension method and I can't imagine how that can hurt. Also note that the _bodyBytes.ToArray() allocation disappears.

I think the use of Span forced the use of continuations in InjectLiveReloadScriptAsync since Span isn't compatible with hoisting in async-await (although things may change over time) but this PR shows that it's not necessary and fixes bugs as a result.

[RickStrahl]

Alright fair enough :smile:

This is just a bonus. 😄 You get a stand-alone, cohesive and reusable extension method and I can't imagine how that can hurt. Also note that the _bodyBytes.ToArray() allocation disappears.

This one was your selling point...

Thanks Atif.