-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Environment variable should override property for gcServer. #61950
Conversation
Add tests for the scenario.
Tagging subscribers to this area: @dotnet/gc Issue DetailsAdd tests for the scenario. Fixes #61674 /cc @cshung @elinor-fung
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for adding the test!
Thanks @AaronRobinsonMSFT for fixing, this feels like something we should queue up for 6.0.x. Thx! |
/backport to release/6.0 |
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1495545849 |
|
||
[Fact] | ||
[ConfigProperty("DOTNET_gcServer", "0")] | ||
static int Verify_ServerGC_Env_Disable(string[] _) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AaronRobinsonMSFT, FYI, this test failed on GCStandAloneServer legs:
- Windows arm64 checked https://helix.dot.net/api/2019-06-17/jobs/1a1caea9-5980-4759-a7d1-da30e1a225a6/workitems/baseservices.RuntimeConfiguration/console
- Windows x64 checked https://helix.dot.net/api/2019-06-17/jobs/6c1bb956-e09d-4509-bc09-9d28f99834cf/workitems/baseservices.RuntimeConfiguration/console.
- Linux arm64 checked https://helix.dot.net/api/2019-06-17/jobs/de826141-187b-4ee7-ba88-31c2637b46af/workitems/baseservices.RuntimeConfiguration/console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cshung this seems like something the GC team should investigate. I am unclear why this would be occurring but given the simplicity of the test I'd wager it is unlikely a test issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mangod9 It looks like the GCStandAloneServer set COMPlus_gcServer=1
, so even when the code set DOTNET_gcServer=0
, we are still unable to disable it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok makes sense. @am11 what CI job was failing due to this issue? Is the failure only after this change or unrelated to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mangod9, it's a new test added by this PR, which started to fail in outerloop test when this PR was merged. Perhaps the root-cause was preexisting and the test has surfaced the problem.
It looks like the GCStandAloneServer set
COMPlus_gcServer=1
, so even when the code setDOTNET_gcServer=0
, we are still unable to disable it.
Based on the config parsing implementation:
runtime/src/coreclr/inc/clrconfignocache.h
Lines 75 to 86 in 4da6b9a
// Priority order is DOTNET_ and then COMPlus_. | |
strcpy_s(nameBuffer, ARRAY_SIZE(nameBuffer), DOTNET_PREFIX_A); | |
fallbackPrefix = COMPLUS_PREFIX_A; | |
} | |
strcat_s(nameBuffer, ARRAY_SIZE(nameBuffer), cfg); | |
LPCSTR val = getEnvFptr != NULL ? getEnvFptr(nameBuffer) : getenv(nameBuffer); | |
if (val == NULL && fallbackPrefix != NULL) | |
{ | |
strcpy_s(nameBuffer, ARRAY_SIZE(nameBuffer), fallbackPrefix); | |
strcat_s(nameBuffer, ARRAY_SIZE(nameBuffer), cfg); |
first
DOTNET_gcServer
will be checked and if the value is NULL
, it will fallback to legacy, COMPlus_gcServer
. It is LPCSTR
, so =0
shouldn't be confused by NULL/0, right?
Fixes #61674
Add tests for the scenario.
The added tests can also be used to validate any of the configuration properties and environment variables used for start-up.
/cc @cshung @elinor-fung
@jkoritzinsky and @trylek Can you please confirm the methodology in this new test works with the system being built?