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

Create workloads enumerator lazily in dotnet-new host #28677

Merged
merged 3 commits into from
Oct 24, 2022

Conversation

JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented Oct 20, 2022

Problem

dotnet new is creating WorkloadInfoHelper that transitively causes creation of 'Microsoft.NET.Workload_.log' log in %temp% folder on each execution (regardles whether workloads constraints are being used or not).
This leads to unnecessary I/O operations. Also under specific circumstances dotnet new can fail due to log already being created (As log names are being created only with per-second granularity: https://github.com/dotnet/sdk/blob/main/src/Cli/dotnet/commands/dotnet-workload/install/NetSdkMsiInstallerClient.cs#L949) :

Unhandled exception: System.IO.IOException: The process cannot access the file 'C:\Users\jankrivanek\AppData\Local\Temp\Microsoft.NET.Workload_20221020_134925.log' because it is being used by another process.
  at Microsoft.Win32.SafeHandles.SafeFileHandle.CreateFile(String fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options)
  at Microsoft.Win32.SafeHandles.SafeFileHandle.Open(String fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
  at System.IO.Strategies.OSFileStreamStrategy..ctor(String path, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
  at System.IO.Strategies.FileStreamHelpers.ChooseStrategyCore(String path, FileMode mode, FileAccess access, FileShare share, FileOptions options, Int64 preallocationSize, Nullable`1 unixCreateMode)
  at System.IO.StreamWriter.ValidateArgsAndOpenPath(String path, Boolean append, Encoding encoding, Int32 bufferSize)
  at System.IO.File.CreateText(String path)
  at Microsoft.DotNet.Installer.Windows.TimestampedFileLogger..ctor(String path, Int32 flushThreshold, String[] logPipeNames)
  at Microsoft.DotNet.Workloads.Workload.Install.NetSdkMsiInstallerClient.Create(Boolean verifySignatures, SdkFeatureBand sdkFeatureBand, IWorkloadResolver workloadResolver, INuGetPackageDownloader nugetPackageDownloader, VerbosityOptions verbosity, PackageSourceLocation packageSourceLocation, IReporter reporter, String tempDirPath, RestoreActionConfig restoreActionConfig)
  at Microsoft.DotNet.Workloads.Workload.Install.WorkloadInstallerFactory.GetWorkloadInstaller(IReporter reporter, SdkFeatureBand sdkFeatureBand, IWorkloadResolver workloadResolver, VerbosityOptions verbosity, String userProfileDir, Boolean verifySignatures, INuGetPackageDownloader nugetPackageDownloader, String dotnetDir, String tempDirPath, PackageSourceLocation packageSourceLocation, RestoreActionConfig restoreActionConfig, Boolean elevationRequired)
  at Microsoft.DotNet.Workloads.Workload.List.WorkloadInfoHelper..ctor(VerbosityOptions verbosity, String targetSdkVersion, Nullable`1 verifySignatures, IReporter reporter, IWorkloadInstallationRecordRepository workloadRecordRepo, String currentSdkVersion, String dotnetDir, String userProfileDir, IWorkloadResolver workloadResolver)
  at Microsoft.DotNet.Cli.NewCommandParser.CreateHost(Boolean disableSdkTemplates, Boolean disableProjectContext, FileInfo projectPath, FileInfo outputPath, LogLevel logLevel)
  at Microsoft.DotNet.Cli.NewCommandParser.<GetCommand>g__GetEngineHost|11_0(ParseResult parseResult)
  at Microsoft.TemplateEngine.Cli.Commands.BaseCommand.CreateEnvironmentSettings(GlobalArgs args, ParseResult parseResult)
  at Microsoft.TemplateEngine.Cli.Commands.BaseCommand`1.InvokeAsync(InvocationContext context)
  at Microsoft.TemplateEngine.Cli.Commands.BaseCommand`1.Invoke(InvocationContext context)
  at System.CommandLine.Invocation.InvocationPipeline.<>c__DisplayClass4_0.<<BuildInvocationChain>b__0>d.MoveNext()

Sample repro

> start /b dotnet new --help & start /b dotnet new --help

System.IO.IOException: The process cannot access the file (...)

Solution

Create the workload provider lazily

Additional details

WorkloadInfoHelper is as well used in dotnet workload commands - so those have the same issue - e.g.:

> start /b dotnet workload list & start /b dotnet workload list

Unhandled exception: System.IO.IOException: The process cannot access the file (...)

however those command or probably much less likely to be executed in parallel or quick sequence

@JanKrivanek JanKrivanek requested a review from a team as a code owner October 20, 2022 12:08
@JanKrivanek JanKrivanek added the Area-dotnet new the item is related to dotnet new command label Oct 20, 2022
@JanKrivanek
Copy link
Member Author

Should we as well right away make sure that logs do not colide (e.g. by adding PID)? https://github.com/dotnet/sdk/blob/main/src/Cli/dotnet/commands/dotnet-workload/install/NetSdkMsiInstallerClient.cs#L949

builtIns.Add((typeof(IWorkloadsInfoProvider), new WorkloadsInfoProvider(new WorkloadInfoHelper())));

builtIns.Add((typeof(IWorkloadsInfoProvider), new WorkloadsInfoProvider(
new Lazy<IWorkloadsRepositoryEnumerator>(() => new WorkloadInfoHelper())))
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider just lazy implementation of IWorkloadsRepositoryEnumerator instead?
just a though

Copy link
Member

@vlada-shubina vlada-shubina left a comment

Choose a reason for hiding this comment

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

The fix looks good from dotnet new standpoint. On the workload side, better logging should be considered (do not fail on logging error, better naming, etc).

@build-analysis build-analysis bot mentioned this pull request Oct 20, 2022
2 tasks
@JanKrivanek JanKrivanek merged commit 1637f34 into dotnet:main Oct 24, 2022
@JanKrivanek JanKrivanek deleted the new-host-fix branch October 24, 2022 14:25
JanKrivanek added a commit to JanKrivanek/sdk that referenced this pull request Oct 24, 2022
Create workloads enumerator lazily in dotnet-new host
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-dotnet new the item is related to dotnet new command Area-Infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants