Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Razor synchronize loader #76357

Merged
merged 9 commits into from
Dec 12, 2024
48 changes: 26 additions & 22 deletions src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,49 +3,53 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Composition;
using System.Diagnostics;
using System.Reflection;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.VisualStudio.Composition;

namespace Microsoft.CodeAnalysis.ExternalAccess.Razor
{
[Export(typeof(IAnalyzerAssemblyResolver)), Shared]
internal class RazorAnalyzerAssemblyResolver : IAnalyzerAssemblyResolver
[method: ImportingConstructor]
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal class RazorAnalyzerAssemblyResolver() : IAnalyzerAssemblyResolver
{
private static Func<AssemblyName, Assembly?>? s_assemblyResolver;

/// <summary>
/// We use this as a heuristic to catch a case where we set the resolver too
/// late and the resolver has already been asked to resolve a razor assembly.
///
/// Note this isn't perfectly accurate but is only used to trigger an assert
/// in debug builds.
/// </summary>
private static bool s_razorRequested = false;
private static readonly List<AssemblyName>? s_assembliesRequested = [];
Copy link
Contributor Author

@chsienki chsienki Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All access to this is done under a lock, and there's only a single instance, so didn't seem worth using an immutable variant or pooling it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this marked as nullable? Are you trying to account for a static constructor race here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a hangover from an earlier version pre-refactoring that I hadn't cleaned up. Fixed.


[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
public RazorAnalyzerAssemblyResolver()
{
}
private static readonly object s_resolverLock = new();

internal static Func<AssemblyName, Assembly?>? AssemblyResolver
/// <summary>
/// Attempts to set the assembly resolver. Will only succeed if the <paramref name="canaryAssembly"/> has not already been requested to load.
chsienki marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
/// <param name="resolver">The resolver function to set.</param>
/// <param name="canaryAssembly">The name of an assembly that is checked to see if it has been requested to load. Setting the resolver will fail if this has already been requested.</param>
/// <returns><c>true</c> when the resolver was successfully set.</returns>
internal static bool TrySetAssemblyResolver(Func<AssemblyName, Assembly?> resolver, AssemblyName canaryAssembly)
{
get => s_assemblyResolver;
set
lock (s_resolverLock)
{
Debug.Assert(s_assemblyResolver == null, "Assembly resolver should not be set multiple times.");
Debug.Assert(!s_razorRequested, "A razor assembly has already been requested before setting the resolver.");
s_assemblyResolver = value;
if (s_assemblyResolver is not null && s_assembliesRequested is not null && !s_assembliesRequested.Contains(canaryAssembly))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how s_assembliesRequested could be null here. Even in a static ctor race s_resolverLock would also be null so we'd throw before this part.

Shouldn't the check be s_assemblyResolver is null? The field starts off as null and this is the only place that it is written to so as stated this will never be set (I believe).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused as to how the s_assemblyResolver can ever become non-null, if entering the block that assigns it requires it to already be non-null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a hangover from an earlier version pre-refactoring that I hadn't cleaned up. Fixed.

{
s_assemblyResolver = resolver;
return true;
}
return false;
}
}

public Assembly? ResolveAssembly(AssemblyName assemblyName)
{
s_razorRequested |= assemblyName.FullName.Contains("Razor");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any potential problems with razor being out of sync with this EA? i.e. razor is older than roslyn, and razor expects the old assembly hook pattern to be used, or roslyn is older than razor, and razor expects the new hook pattern.

Maybe we just have to take care to dual insert and we'll be ok?

return s_assemblyResolver?.Invoke(assemblyName);
lock (s_resolverLock)
{
s_assembliesRequested?.Add(assemblyName);
return s_assemblyResolver?.Invoke(assemblyName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is doing a callback inside of a lock. That usually gives me a bit of pause. Is there a specific reason it needs to be inside the lock? Did you consider say saving the field to a local and then invoking that outside the lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we need to retrieve the resolver under the lock, but once we've done that I can't think of any reason why we need to actually execute it under the lock.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no concern about the resolver being invoked on different threads without synchronization, right? Just a need for the s_assembliesRequested update and s_assemblyResolver read to be synchronized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it, we don't even need to retrieve it under the lock. Before we enter the lock we either 1) have the resolver set or 2) we don't:

In the case of 1) the locking doesn't actually have any effect.

In the case of 2) if the assembly being requested is the canary assembly, then once we exit the lock the resolver can never be set, so it's also safe to just use it.

In the case of 2) where it's not the canary assembly being requested, we actually have a race condition. We exit the lock having added the load to the list, but the state of the resolver can still change. If the install call comes in in the time between exiting the lock and executing the resolver we'll use the resolver, if it doesn't we wont. Now, for our code this actually doesn't matter, really, we only want to execute the resolver for the canary assembly anyway (but we can't do that because we don't know ahead of time what it is).

We can make the last part deterministic by retrieving the loader under the lock and executing against the local value, but don't need to for it to be correct for our usage. I'm leaning towards making it deterministic just for ease-of-understanding the code though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up just making this deterministic in all cases so its easier to reason about.

}
}
}
}
9 changes: 9 additions & 0 deletions src/Tools/ExternalAccess/Razor/RazorProjectExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,14 @@ internal static class RazorProjectExtensions
{
return project.GetSourceGeneratorRunResultAsync(cancellationToken);
}

internal static async ValueTask<IEnumerable<SourceGeneratedDocument>> GetSourceGeneratedDocumentsForGeneratorAsync(this Project project, string assemblyName, string assemblyPath, Version assemblyVersion, string typeName, CancellationToken cancellationToken)
{
var results = await project.GetSourceGeneratedDocumentsAsync(cancellationToken).ConfigureAwait(false);

var generatorIdentity = new SourceGeneratorIdentity(assemblyName, assemblyPath, assemblyVersion, typeName);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SourceGeneratorIdentity isn't public, so have to pass through the requisite parts. We could make an IVT copy but doesn't seem worth it.

return results.Where(s => s.Identity.Generator == generatorIdentity);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra blank line

}
}
}
Loading