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.

Sign up for GitHub

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

Jump to bottom

Static graph-based restore crashes on systems with alternate console encodings #12373

Closed
jeffkl opened this issue Jan 18, 2023 · 2 comments · Fixed by NuGet/NuGet.Client#5010
Closed

Static graph-based restore crashes on systems with alternate console encodings #12373

jeffkl opened this issue Jan 18, 2023 · 2 comments · Fixed by NuGet/NuGet.Client#5010
Assignees
@jeffkl
Labels
Area:RestoreStaticGraph Issues related to the Static Graph restore Functionality:Restore Priority:1 High priority issues that must be resolved in the current sprint. RegressionFromPreviousRTM A regression from the last RTM. Example: worked in 6.2, doesn't work in 6.3 Type:Bug
Milestone
6.6

Comments

@jeffkl
Copy link
Contributor

jeffkl commented Jan 18, 2023 •
edited
Loading

NuGet Product Used

dotnet.exe, MSBuild.exe

Product Version

Visual Studio 17.5

Worked before?

Yes, Visual Studio 17.4

Impact

It's more difficult to complete my work

Repro Steps & Context

In Windows Region settings, enable "Use Unicode UTF-8 for worldwide language support"

image

This seems to make Console.StandardInput have a 3-byte preamble, causing the logic that read the static graph restore arguments to not work correctly.

Reported by @vlada-shubina

Caused by NuGet/NuGet.Client#4772

This is fixed in 17.6

Workaround

Disable the Windows Region setting and reboot.

Verbose Logs

No response
@jeffkl jeffkl added Priority:1 High priority issues that must be resolved in the current sprint. Type:Bug Functionality:Restore RegressionFromPreviousRTM A regression from the last RTM. Example: worked in 6.2, doesn't work in 6.3 Area:RestoreStaticGraph Issues related to the Static Graph restore labels Jan 18, 2023
@jeffkl jeffkl self-assigned this Jan 18, 2023
@jeffkl jeffkl mentioned this issue Jan 18, 2023
Merged
8 tasks
jaredpar added a commit to jaredpar/roslyn that referenced this issue Jan 26, 2023
@jaredpar
Work around NuGet static graph restore bug
9a6689e 
NuGet/Home#12373
@MattGal MattGal mentioned this issue Jan 26, 2023
Closed
jaredpar pushed a commit to dotnet/roslyn that referenced this issue Jan 26, 2023
Don't overwrite binary logs (#66559)
66cf7ab 
* Don't overwrite binary logs

Several of our legs were calling `build.ps1 -binaryLog` in a row and
that re-uses the binary log file name which causes an overwrite. This
change allows us to specify the name and adds a warning if we do
accidentally overwrite in the future.

* enable trace for diff issue

* Work around NuGet static graph restore bug

NuGet/Home#12373
@jaredpar jaredpar mentioned this issue Feb 14, 2023
Closed
@AR-May AR-May mentioned this issue Feb 24, 2023
Merged
AR-May added a commit to dotnet/msbuild that referenced this issue Feb 24, 2023
@AR-May
Turn off static graph restore. (#8498)
7c4d75d 
Our CI builds fails because of bug NuGet/Home#12373. 
It is fixed in NuGet/NuGet.Client#5010. 
We are waiting for it to flow to CI machines. Meanwhile this PR applies a workaround.

Note: This PR needs to be reverted once it happens.
@jeffkl
Copy link
Contributor Author

jeffkl commented Mar 8, 2023

Reopening as the original fix isn't good enough. In some scenarios the STDIN encoding by the code that creates the process sees it as UTF8 while the process itself sees SBSCCodePageEncoding.

image

@jeffkl jeffkl reopened this Mar 8, 2023
@jeffkl jeffkl added this to the 6.6 milestone Mar 8, 2023
rokonec pushed a commit to rokonec/msbuild that referenced this issue Mar 10, 2023
@AR-May @rokonec
Turn off static graph restore. (dotnet#8498)
18c8529 
Our CI builds fails because of bug NuGet/Home#12373. 
It is fixed in NuGet/NuGet.Client#5010. 
We are waiting for it to flow to CI machines. Meanwhile this PR applies a workaround.

Note: This PR needs to be reverted once it happens.
rokonec pushed a commit to rokonec/msbuild that referenced this issue Mar 14, 2023
@AR-May @rokonec
Turn off static graph restore. (dotnet#8498)
f0f039c 
Our CI builds fails because of bug NuGet/Home#12373. 
It is fixed in NuGet/NuGet.Client#5010. 
We are waiting for it to flow to CI machines. Meanwhile this PR applies a workaround.

Note: This PR needs to be reverted once it happens.
rokonec added a commit to dotnet/msbuild that referenced this issue Mar 14, 2023
@rokonec @AR-May
Concurrency bug fix - BuildManager instances acquire its own BuildTel…
f08e881 
…emetry instance (#8561)

* BuildManager instances acquire its own BuildTelemetry instance (#8444)

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1708215

Context
In VS there are multiple instances of BuildManager called asynchronously. For DTB and normal build and maybe other which I have not identified yet.

Changes Made
BuildManager instances acquire its own BuildTelemetry instance as oppose to sharing single BuildTelemetry instance in non thread safe manner.

Testing
Locally
# Conflicts:
#	src/Build/BackEnd/Client/MSBuildClient.cs - resolved with minimal and safe approach

* Bumping version

* Turn off static graph restore. (#8498)

Our CI builds fails because of bug NuGet/Home#12373. 
It is fixed in NuGet/NuGet.Client#5010. 
We are waiting for it to flow to CI machines. Meanwhile this PR applies a workaround.

Note: This PR needs to be reverted once it happens.

---------

Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>
allisonchou pushed a commit to allisonchou/roslyn that referenced this issue Mar 20, 2023
@allisonchou
Don't overwrite binary logs (dotnet#66559)
98b5b46 
* Don't overwrite binary logs

Several of our legs were calling `build.ps1 -binaryLog` in a row and
that re-uses the binary log file name which causes an overwrite. This
change allows us to specify the name and adds a warning if we do
accidentally overwrite in the future.

* enable trace for diff issue

* Work around NuGet static graph restore bug

NuGet/Home#12373
rokonec added a commit to dotnet/msbuild that referenced this issue Mar 27, 2023
@dotnet-maestro-bot @rokonec @AR-May
[automated] Merge branch 'vs17.4' => 'vs17.5' (#8562)
8b68842 
* Concurrency bug fix - BuildManager instances acquire its own BuildTelemetry instance (#8561)

* BuildManager instances acquire its own BuildTelemetry instance (#8444)

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1708215

Context
In VS there are multiple instances of BuildManager called asynchronously. For DTB and normal build and maybe other which I have not identified yet.

Changes Made
BuildManager instances acquire its own BuildTelemetry instance as oppose to sharing single BuildTelemetry instance in non thread safe manner.

Testing
Locally
# Conflicts:
#	src/Build/BackEnd/Client/MSBuildClient.cs - resolved with minimal and safe approach

* Bumping version

* Turn off static graph restore. (#8498)

Our CI builds fails because of bug NuGet/Home#12373. 
It is fixed in NuGet/NuGet.Client#5010. 
We are waiting for it to flow to CI machines. Meanwhile this PR applies a workaround.

Note: This PR needs to be reverted once it happens.

---------

Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>

* Use AutoResetEvent as oppose to ManualResetEventSlim (#8575)

Summary
Customer, mainly internal like XStore, with huge repos, using msbuild /graph /bl on powerful development and build computers, might experience 15x plus regression in evaluation time.

It has been identified as performance bug in our logging event pub/sub mechanism. When ingest queue reaches its bound, at .net472 ManualResetEventSlim causes way too many thread.Yields flooding the system with thread context switches.
This hypothesis has been verified by PerfMon perfcounter System.ContextSwitches.

Alhougt counterintuitive, AutoResetEvent , ManualResetEvent or even SpinLocking produced better behavior and with those the issue no longer reproduce.

Customer Impact
In case of XStore it was about 7 minutes in build time.

Regression?
Yes, introduced in VS 17.4.

Testing
Manual validation by @rokonec and automated tests. Using local repro to verify changes has fixed it.

Risk
Low

Note
It effect only VS MSBuild.exe. In dotnet build ManualResetEventSlim works better.

---------

Co-authored-by: Roman Konecny <rokonecn@microsoft.com>
Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>
@jeffkl jeffkl modified the milestone: 6.6 Apr 3, 2023
@jeffkl
Copy link
Contributor Author

jeffkl commented Apr 6, 2023

This is fixed in 17.6 which will be generally available soon. The only workaround for 17.5 is to disable static graph-based restore.

@jeffkl jeffkl closed this as completed Apr 6, 2023
dibarbet pushed a commit to dotnet/roslyn that referenced this issue Apr 19, 2023
@dibarbet
Don't overwrite binary logs (#66559)
c269944 
* Don't overwrite binary logs

Several of our legs were calling `build.ps1 -binaryLog` in a row and
that re-uses the binary log file name which causes an overwrite. This
change allows us to specify the name and adds a warning if we do
accidentally overwrite in the future.

* enable trace for diff issue

* Work around NuGet static graph restore bug

NuGet/Home#12373
@Youssef1313 Youssef1313 mentioned this issue Sep 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jeffkl jeffkl

Labels
Area:RestoreStaticGraph Issues related to the Static Graph restore Functionality:Restore Priority:1 High priority issues that must be resolved in the current sprint. RegressionFromPreviousRTM A regression from the last RTM. Example: worked in 6.2, doesn't work in 6.3 Type:Bug
Projects
None yet
Milestone
6.6
Development

Successfully merging a pull request may close this issue.

Properly handle alternate console stream encodings in StaticGraphRestoreArguments NuGet/NuGet.Client
1 participant
@jeffkl