Skip to content

Create workloads enumerator lazily in dotnet-new host#28677

Merged
JanKrivanek merged 3 commits intodotnet:mainfrom
JanKrivanek:new-host-fix
Oct 24, 2022
Merged

Create workloads enumerator lazily in dotnet-new host#28677
JanKrivanek merged 3 commits intodotnet:mainfrom
JanKrivanek:new-host-fix

Conversation

@JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented Oct 20, 2022 •
edited
Loading

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
@ghost ghost added the Area-Infrastructure label Oct 20, 2022
@JanKrivanek JanKrivanek added the Area-dotnet new the item is related to dotnet new command label Oct 20, 2022
@build-analysis build-analysis bot mentioned this pull request Oct 20, 2022
Closed
2 tasks
@JanKrivanek
Copy link
Member Author

JanKrivanek commented Oct 20, 2022

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

vlada-shubina
vlada-shubina reviewed Oct 20, 2022
View reviewed changes
src/Cli/dotnet/commands/dotnet-new/NewCommandParser.cs
builtIns.Add((typeof(IWorkloadsInfoProvider), new WorkloadsInfoProvider(new WorkloadInfoHelper())));

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

@vlada-shubina vlada-shubina Oct 20, 2022

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

vlada-shubina
vlada-shubina approved these changes Oct 20, 2022
View reviewed changes
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
Closed
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
@JanKrivanek
Merge pull request dotnet#28677 from JanKrivanek/new-host-fix
7de5a9f 
Create workloads enumerator lazily in dotnet-new host
@JanKrivanek JanKrivanek mentioned this pull request Oct 24, 2022
Merged
@YuliiaKovalova YuliiaKovalova mentioned this pull request Feb 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vlada-shubina vlada-shubina vlada-shubina approved these changes

Assignees

No one assigned

Labels

Area-dotnet new the item is related to dotnet new command Area-Infrastructure

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JanKrivanek @vlada-shubina