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

Fix race condition in ServiceProcess tests #59676

Merged
merged 1 commit into from
Sep 28, 2021

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Sep 27, 2021

Fixes #59297

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

LGTM, thank you. Why did this take so long to appear I wonder.

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@ericstj
Copy link
Member Author

ericstj commented Sep 27, 2021

/azp run outerloop

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@ericstj
Copy link
Member Author

ericstj commented Sep 27, 2021

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danmoseley
Copy link
Member

@BruceForstall should the format job be clean? also, it has a bunch of errors like

Running: clang-tidy -fix -checks=-*,readability-braces*,modernize-use-nullptr  -header-filter=jit/.* -p=D:\a\1\s\src\coreclr\..\..\artifacts\obj\coreclr\windows.x64.Release\compile_commands.json D:\a\1\s\src\coreclr\jit\alloc.cpp
Unhandled exception. System.ComponentModel.Win32Exception (193): The specified executable is not a valid application for this OS platform.

@danmoseley
Copy link
Member

Arm32 failures are known: #58481

@danmoseley danmoseley merged commit da358db into dotnet:main Sep 28, 2021
@danmoseley
Copy link
Member

@ericstj do the outerloop tests even run as admin (so these tests actually run)?

I actually can't find the config in the Azdo UI that is outerloop. Looking at each of the Windows libraries test configs, I see a line like this:

2021-09-27T22:48:26.1877349Z ./build.sh -subset libs.pretest -rc release -configuration Debug -ci -arch x64 -framework net7.0  -testscope innerloop  /p:RuntimeOS=linux-musl   /p:RuntimeArtifactsPath=/home/vsts/work/1/s/artifacts/transport/coreclr  /p:RuntimeFlavor=coreclr /bl:/home/vsts/work/1/s/artifacts/log/Debug/overrideRuntimeFromLiveDrop.binlog

Note -testscope innerloop. So I'm not sure where the outerloop ones are, so I couldn't try to figure out if they run elevated.

@ericstj
Copy link
Member Author

ericstj commented Sep 28, 2021

CHecking the logs. @safern recommended this trigger to me. Not sure if the Test UI is good for checking on passing tests because there are so many. I might be able to find the console logs from build log spew.

@ericstj
Copy link
Member Author

ericstj commented Sep 28, 2021

I actually don't see where libraries outerloop tests are even run. I see coreclr runtime pri1, but no libraries tests.

@safern
Copy link
Member

safern commented Sep 28, 2021

Oops this was my bad, there is a runtime-coreclr outerloop and a runtime-libraries-coreclr outerloop and when I searched for outerloop I saw both but when telling @ericstj the command to run it I accidentally missed the -libraries portion of the pipeline name 😞 . Sorry about that.

@ericstj
Copy link
Member Author

ericstj commented Sep 28, 2021

No worries, next time I'll use /azp run runtime-libraries-coreclr outerloop-windows

@danmoseley
Copy link
Member

Good info. @safern do you know (or know how to know) whether we run elevated?

@safern
Copy link
Member

safern commented Sep 28, 2021

@safern do you know (or know how to know) whether we run elevated?

IIRC helix do run elevated, but I'd like @MattGal to confirm that to be on the safe side.

@ericstj
Copy link
Member Author

ericstj commented Sep 28, 2021

I opened #59712 to find out (and get test coverage I was hoping to have in this PR). Interestingly since its the same hash it doesn't retrigger the main validation builds, bonus resource savings :)

@MattGal
Copy link
Member

MattGal commented Sep 28, 2021

@safern do you know (or know how to know) whether we run elevated?

IIRC helix do run elevated, but I'd like @MattGal to confirm that to be on the safe side.

Yes, you have the ability to start processes lower but the default is prompt-less elevated. In the earliest days, we had some machines set up to run the helix client in Medium IL mode and it hit tons of problems with people's work items.

@ericstj
Copy link
Member Author

ericstj commented Sep 28, 2021

🧬 == 🍯

@ericstj
Copy link
Member Author

ericstj commented Sep 28, 2021

@MattGal were those issues in regular runs or just outerloop? I remember at one point we said outerloop would get elevation but inner loop wouldn't.

@MattGal
Copy link
Member

MattGal commented Sep 28, 2021

@MattGal were those issues in regular runs or just outerloop? I remember at one point we said outerloop would get elevation but inner loop wouldn't.

The helix agents don't know or care about your concepts of inner/outer-loopiness.

@danmoseley
Copy link
Member

Yes, you have the ability to start processes lower but the default is prompt-less elevated.

How is this done? It might be good to add one such run in the mix.

Also, is this the case for both Windows and Unix (elevated or su respectively)

@BruceForstall
Copy link
Member

@BruceForstall should the format job be clean? also, it has a bunch of errors like

Running: clang-tidy -fix -checks=-*,readability-braces*,modernize-use-nullptr  -header-filter=jit/.* -p=D:\a\1\s\src\coreclr\..\..\artifacts\obj\coreclr\windows.x64.Release\compile_commands.json D:\a\1\s\src\coreclr\jit\alloc.cpp
Unhandled exception. System.ComponentModel.Win32Exception (193): The specified executable is not a valid application for this OS platform.

It should be, and is in subsequent runs.

The real failure was earlier:

Invoke-WebRequest : Unable to read data from the transport connection: An existing connection was forcibly closed by 
the remote host.
At D:\a\1\s\eng\formatting\download-tools.ps1:20 char:23
+ ...   $status = Invoke-WebRequest -Uri "$baseUri/$toolName.exe" -OutFile  ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [Invoke-WebRequest], IOException
    + FullyQualifiedErrorId : System.IO.IOException,Microsoft.PowerShell.Commands.InvokeWebRequestCommand

@jkoritzinsky Is this your new downloader from #59374 not handling failure well?

@jkoritzinsky
Copy link
Member

Yeah I think there’s a problem with the new downloader script. I’ll take a look into it next time I have cycles

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure System.ServiceProcess.Tests.ServiceBaseTests.PropagateExceptionFromOnStart
6 participants