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

Environment variable should override property for gcServer. #61950

Merged
merged 1 commit into from
Nov 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/coreclr/utilcode/clrconfig.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ BOOL CLRConfig::IsConfigEnabled(const ConfigDWORDInfo & info)
//
// Arguments:
// * info - see file:../inc/CLRConfig.h for details.
//
// * isDefault - the value was not set or had an invalid format so the default was returned.
// * result - the result.
//
// Return value:
Expand All @@ -449,9 +449,7 @@ DWORD CLRConfig::GetConfigValue(const ConfigDWORDInfo & info, /* [Out] */ bool *

DWORD resultMaybe;
HRESULT hr = GetConfigDWORD(info.name, info.defaultValue, &resultMaybe, info.options);

// Ignore the default value even if it's set explicitly.
if (resultMaybe != info.defaultValue)
if (SUCCEEDED(hr))
{
*isDefault = false;
return resultMaybe;
Expand Down
159 changes: 159 additions & 0 deletions src/tests/baseservices/RuntimeConfiguration/TestConfig.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Text;
using System.Reflection;
using System.Runtime;

using Xunit;

class TestConfig
{
const int Success = 100;
const int Fail = 101;

[Fact]
[EnvVar("DOTNET_gcServer", "1")]
static int Verify_ServerGC_Env_Enable(string[] _)
{
return GCSettings.IsServerGC
? Success
: Fail;
}

[Fact]
[ConfigProperty("DOTNET_gcServer", "0")]
static int Verify_ServerGC_Env_Disable(string[] _)
Copy link
Member

@am11 am11 Nov 26, 2021

Choose a reason for hiding this comment

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

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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 set DOTNET_gcServer=0, we are still unable to disable it.

Based on the config parsing implementation:

// 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?

{
return GCSettings.IsServerGC
? Fail
: Success;
}

[Fact]
[ConfigProperty("System.GC.Server", "true")]
static int Verify_ServerGC_Prop_Enable(string[] _)
{
return GCSettings.IsServerGC
? Success
: Fail;
}

[Fact]
[ConfigProperty("System.GC.Server", "false")]
static int Verify_ServerGC_Prop_Disable(string[] _)
{
return GCSettings.IsServerGC
? Fail
: Success;
}

[Fact]
[EnvVar("DOTNET_gcServer", "0")]
[ConfigProperty("System.GC.Server", "true")]
static int Verify_ServerGC_Env_Override_Prop(string[] _)
{
return GCSettings.IsServerGC
? Fail
: Success;
}

static int Main(string[] args)
{
if (args.Length == 0)
{
return RunTests();
}

MethodInfo infos = typeof(TestConfig).GetMethod(args[0], BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public);
if (infos is null)
{
return Fail;
}
return (int)infos.Invoke(null, new object[] { args[1..] });
}

[AttributeUsage(AttributeTargets.Method, AllowMultiple = true, Inherited = false)]
class EnvVarAttribute : Attribute
{
public EnvVarAttribute(string name, string value) { Name = name; Value = value; }
public string Name { get; init; }
public string Value { get; init; }
}

[AttributeUsage(AttributeTargets.Method, AllowMultiple = true, Inherited = false)]
class ConfigPropertyAttribute : Attribute
{
public ConfigPropertyAttribute(string name, string value) { Name = name; Value = value; }
public string Name { get; init; }
public string Value { get; init; }
}

static int RunTests()
{
string corerunPath = GetCorerunPath();
MethodInfo[] infos = typeof(TestConfig).GetMethods(BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public);
foreach (var mi in infos)
{
var factMaybe = mi.GetCustomAttributes(typeof(FactAttribute));
if (!factMaybe.Any())
{
continue;
}

using Process process = new();

StringBuilder arguments = new();
var configProperties = mi.GetCustomAttributes(typeof(ConfigPropertyAttribute));

foreach (Attribute cp in configProperties)
{
ConfigPropertyAttribute configProp = (ConfigPropertyAttribute)cp;
arguments.Append($"-p {configProp.Name}={configProp.Value} ");
}

arguments.Append($"\"{System.Reflection.Assembly.GetExecutingAssembly().Location}\" {mi.Name}");

process.StartInfo.FileName = corerunPath;
process.StartInfo.Arguments = arguments.ToString();

var envVariables = mi.GetCustomAttributes(typeof(EnvVarAttribute));
foreach (string key in Environment.GetEnvironmentVariables().Keys)
{
process.StartInfo.EnvironmentVariables[key] = Environment.GetEnvironmentVariable(key);
}

Console.WriteLine($"Running: {process.StartInfo.Arguments}");
foreach (Attribute ev in envVariables)
{
EnvVarAttribute envVar = (EnvVarAttribute)ev;
process.StartInfo.EnvironmentVariables[envVar.Name] = envVar.Value;
Console.WriteLine($" set {envVar.Name}={envVar.Value}");
}

process.Start();
process.WaitForExit();
if (process.ExitCode != Success)
{
Console.WriteLine($"Failed: {mi.Name}");
return process.ExitCode;
}
}

return Success;
}

static string GetCorerunPath()
{
string corerunName = "corerun";
if (TestLibrary.Utilities.IsWindows)
{
corerunName += ".exe";
}
return Path.Combine(Environment.GetEnvironmentVariable("CORE_ROOT"), corerunName);
}
}
15 changes: 15 additions & 0 deletions src/tests/baseservices/RuntimeConfiguration/TestConfig.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<!-- This test provides no interesting scenarios for GCStress -->
<GCStressIncompatible>true</GCStressIncompatible>
<UnloadabilityIncompatible>true</UnloadabilityIncompatible>
<DisableProjectBuild Condition="'$(RuntimeFlavor)' == 'Mono'">true</DisableProjectBuild>
</PropertyGroup>
<ItemGroup>
<Compile Include="TestConfig.cs" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="$(TestSourceDir)Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
</ItemGroup>
</Project>
3 changes: 3 additions & 0 deletions src/tests/issues.targets
Original file line number Diff line number Diff line change
Expand Up @@ -876,6 +876,9 @@
<ExcludeList Include="$(XunitTestBinBase)/baseservices/TieredCompilation/BasicTestWithMcj/*">
<Issue>Tests features specific to coreclr</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/baseservices/RuntimeConfiguration/*">
<Issue>Tests features specific to coreclr</Issue>
</ExcludeList>
<ExcludeList Include="$(XunitTestBinBase)/Loader/CollectibleAssemblies/ByRefLocals/**">
<Issue>https://github.com/dotnet/runtime/issues/40394</Issue>
</ExcludeList>
Expand Down