Razorlight: InplaceStringBuilder no longer exists in Microsoft.Extensions.Primitives

[jamesgurung]

Template compilation now fails because the obsoleted InplaceStringBuilder was removed from the .NET Runtime in commit https://github.com/dotnet/runtime/commit/ef4bc4d503b71f9de1a8988da6db38bcfcfb0cc4.

This causes an error on the line:

https://github.com/toddams/RazorLight/blob/8e043e10423ecc0b88b49787dd597adddd4caa31/src/RazorLight/Compilation/RazorTemplateCompiler.cs#L263

System.TypeLoadException: Could not load type 'Microsoft.Extensions.Primitives.InplaceStringBuilder' from assembly 'Microsoft.Extensions.Primitives, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'.
at RazorLight.Compilation.RazorTemplateCompiler.NormalizeKey(String templateKey)
at RazorLight.Compilation.RazorTemplateCompiler.GetNormalizedKey(String templateKey)
at RazorLight.Compilation.RazorTemplateCompiler.CompileAsync(String templateKey)
at RazorLight.EngineHandler.CompileTemplateAsync(String key)
at RazorLight.EngineHandler.CompileRenderAsyncT

[jzabroski]

Thanks for mentioning this. Do you want to submit a PR?

[jamesgurung]

I'm afraid I don't know enough about string allocations to submit a PR. Changing the type to StringBuilder will compile, but I'm not convinced that would be any more efficient than:

c# return (addLeadingSlash ? "/" : string.Empty) + templateKey.Replace('\\', '/');

[jzabroski]

1. Add test coverage for NormalizeKey for EmbeddedRazorProject (trivial - this is identity function test - you can use a Guid.NewGuid + "." + Guid.NewGuid as your key to test )
2. Add test coverage for NormalizeKey for FileSystemRazorProject (this requires more work - what it's trying to do is normalize unix and windows file path conventions so that if somebody accidentally uses a backslash, we convert it to a forward slash.)
3. I think there are two possible ways to implement this:
   
   
   
   1. Solve the problem by avoiding it (and introducing file system access): Since .NET Framework 4.6.2, on Windows, .NET Framework use the OS function GetFullPathName. This same function used internally by the runtime framework is also exposed as an API call: https://docs.microsoft.com/en-us/dotnet/api/system.io.path.getfullpath?view=netcore-3.1#System_IO_Path_GetFullPath_System_String_ - as the remark suggestion suggests, the downside to this is it requires file system access to compute the full path name. However, it will return a canonical path name. The downside to calling this function is that someone could change the casing of the file, and so two calls to Path.GetFullPath could return two different values for the same logical resource. (I think this is bad, so we may just want to update the code to explicitly document the reasons for not calling Path.GetFullPath).
   
   
   2. Rather than use hardcoded values for '\' and '/', we should probably use canonical system.io.path.directoryseparatorchar and system.io.path.altdirectoryseparatorchar - these have specific per-runtime os pair configuration values that make sense for the target platform. Thoughts?
   
   
   3. It's probably sufficient to just replace InlineStringBuilder with StringBuilder. The only scenario I can see where InlineStringBuilder would be useful is in things like video games that try to minimize GC pauses due to intermediary allocations.
   
   

[jzabroski]

@HamiltonManalo is this something you'd be interested in fixing for .NET Core 5.0?

[HamiltonManalo]

@jzabroski Yeah, I will start work on this as soon as I can.

[HamiltonManalo]

For the InplaceStringBuilder replacement there is
string.Create
below is an article using it.
https://www.stevejgordon.co.uk/creating-strings-with-no-allocation-overhead-using-string-create-csharp

Have been fighting off a flu/sinusitis this week so I wasn't active. I'll look deeper, but my initial feeling is that unless this is sending thousands of emails a minute, the allocations aren't a huge deal.

[georgeemr]

+1

[HamiltonManalo]

Hopefully this weekend I'll be able to free up some time and focus on this so we can close the issue.

[kkkmail]

The same error seems to be coming from this point as well:

fail: Microsoft.AspNetCore.Server.Kestrel[0]
      Heartbeat.OnHeartbeat
      System.TypeLoadException: Could not load type 'Microsoft.Extensions.Primitives.InplaceStringBuilder' from assembly 'Microsoft.Extensions.Primitives, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'.
         at Microsoft.Net.Http.Headers.DateTimeFormatter.ToRfc1123String(DateTimeOffset dateTime, Boolean quoted)
         at Microsoft.Net.Http.Headers.HeaderUtilities.FormatDate(DateTimeOffset dateTime, Boolean quoted)
         at Microsoft.Net.Http.Headers.HeaderUtilities.FormatDate(DateTimeOffset dateTime)
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.DateHeaderValueManager.SetDateValues(DateTimeOffset value)
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.DateHeaderValueManager.OnHeartbeat(DateTimeOffset now)
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.Heartbeat.OnHeartbeat()
info: CoreWCF.Channels.ServiceModelHttpMiddleware[0]

This is with NET 5 RC2 today. I am not sure if this is related though... So, please, disregard if it is not.

[HamiltonManalo]

@kkkmail A fix hasn't been implemented for this yet. I'm going to begin working on it tonight (better late than never) and it should make it into the Beta11 release.

[ADNewsom09]

I created a quick simple PR that changes to using StringBuilder. #380

[Pete-PlaytimeSolutions]

I noticed that the fix for this has already been merged. Will the next Beta be available soon?