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

#4353 Config.WithFallback is acting inconsistently #4358

Merged
merged 9 commits into from
Mar 25, 2020
Merged

#4353 Config.WithFallback is acting inconsistently #4358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b7cc0d7
Add bug spec
Arkatufus Mar 25, 2020
52d166e
Missing keys during merge
Arkatufus Mar 25, 2020
c9ac33c
Remove weird workaround that doesn't make any sense
Arkatufus Mar 25, 2020
0d2bf60
How root are calculated neeed to be reversed
Arkatufus Mar 25, 2020
3223e6b
Original node value need to be preserved
Arkatufus Mar 25, 2020
4d5d60a
use Config.Root to standardize Config access
Arkatufus Mar 25, 2020
4695e87
Possible fix for "collection was modified" exception during merge
Arkatufus Mar 25, 2020
e3b1800
Minor optimization
Arkatufus Mar 25, 2020
8ce4683
Merge branch 'dev' into #4353_Akka.Remote_port_config_reverts_to_default
Arkatufus Mar 25, 2020
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
146 changes: 146 additions & 0 deletions src/core/Akka.Cluster.Tests.MultiNode/Bugfix4353Specs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
using System;
using System.Linq;
using System.Collections.Generic;
using System.Threading;

using Akka.Actor;
using Akka.Cluster.TestKit;
using Akka.Configuration;
using Akka.Remote.TestKit;
using Akka.TestKit;

namespace Akka.Cluster.Tests.MultiNode
{
public class Bugfix4353Spec : MultiNodeClusterSpec
{
protected RoleName First;
protected RoleName Second;
protected RoleName Third;

public Bugfix4353Spec() : this(new Bugfix4353SpecsConfig())
{
}

protected Bugfix4353Spec(Bugfix4353SpecsConfig config) : base(config, typeof(Bugfix4353Spec))
{
First = config.First;
Second = config.Second;
Third = config.Third;
}

[MultiNodeFact]
public void Bugfix4353Spec_Cluster_of_3_must_reach_cnovergence()
{
AwaitClusterUp(First, Second, Third);
EnterBarrier("after-1");
}
}

public class Bugfix4353SpecsConfig : MultiNodeConfig
{
private static readonly string[] Hocons =
{
@"akka : {
actor : {
provider : cluster
}
}",

@"akka : {
stdout-loglevel : INFO
loglevel : INFO
log-config-on-start : on
loggers : [""Akka.Event.DefaultLogger""],
actor : {
debug : {
receive : on
autoreceive : on
lifecycle : on
event-stream : on
unhandled : on
}
}
}",

@"akka : {
remote : {
dot-netty : {
tcp : {
log-transport : true
transport-class : ""Akka.Remote.Transport.DotNetty.TcpTransport, Akka.Remote""
transport-protocol : tcp
hostname : 0.0.0.0
public-hostname : localhost
}
}
}
}",

@"akka : {
cluster : {
log-info : on
seed-nodes : [
""akka.tcp://Bugfix4353Spec@localhost:5001"",
""akka.tcp://Bugfix4353Spec@localhost:5002"",
""akka.tcp://Bugfix4353Spec@localhost:5003""
]
roles : [seed]
role : { }
}
}"
};

public static Config Config
{
get
{
var config = ConfigurationFactory.Empty;

foreach (var hocon in Hocons)
{
config = config.WithFallback(ConfigurationFactory.ParseString(hocon));
}
return config;
}
}

public readonly RoleName First;
public readonly RoleName Second;
public readonly RoleName Third;

public Bugfix4353SpecsConfig()
{

First = Role("first");
Second = Role("second");
Third = Role("third");

CommonConfig = MultiNodeClusterSpec.ClusterConfig(false);

NodeConfig(
new[] { First },
new[] {
ConfigurationFactory
.ParseString("akka.remote.dot-netty.tcp.port : 5001")
.WithFallback(Config)
});

NodeConfig(
new[] { Second },
new[] {
ConfigurationFactory
.ParseString("akka.remote.dot-netty.tcp.port : 5002")
.WithFallback(Config)
});

NodeConfig(
new[] { Third },
new[] {
ConfigurationFactory
.ParseString("akka.remote.dot-netty.tcp.port : 5003")
.WithFallback(Config)
});

}
}
}
2 changes: 1 addition & 1 deletion src/core/Akka/Actor/Deploy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public bool Equals(Deploy other)
string.Equals(_path, other._path) &&
_routerConfig.Equals(other._routerConfig) &&
((_config.IsNullOrEmpty() && other._config.IsNullOrEmpty()) ||
_config.ToString().Equals(other._config.ToString())) &&
_config.Root.ToString().Equals(other._config.Root.ToString())) &&
(_scope == null && other._scope == null || (_scope != null && _scope.Equals(other._scope)));
}

Expand Down
2 changes: 1 addition & 1 deletion src/core/Akka/Actor/Settings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ private static string GetProviderClass(string provider)
/// <inheritdoc/>
public override string ToString()
{
return Config.ToString();
return Config.Root.ToString();
Aaronontheweb marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
30 changes: 12 additions & 18 deletions src/core/Akka/Configuration/Config.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public Config(HoconRoot root)
if (root.Value == null)
throw new ArgumentNullException(nameof(root), "The root value cannot be null.");

Value = root.Value;
Root = root.Value;
Substitutions = root.Substitutions;
}
Expand All @@ -52,6 +53,7 @@ public Config(Config source, Config fallback)
if (source == null)
throw new ArgumentNullException(nameof(source), "The source configuration cannot be null.");

Value = source.Value;
Root = source.Root;
Fallback = fallback;
}
Expand All @@ -69,6 +71,8 @@ public virtual bool IsEmpty
get { return Root == null || Root.IsEmpty; }
}

private HoconValue Value { get; set; }

/// <summary>
/// The root node of this configuration section
/// </summary>
Expand All @@ -90,6 +94,7 @@ protected Config Copy(Config fallback = null)
{
Fallback = Fallback != null ? Fallback.Copy(fallback) : fallback,
Root = Root,
Value = Value,
Substitutions = Substitutions
};
}
Expand Down Expand Up @@ -423,10 +428,10 @@ public virtual TimeSpan GetTimeSpan(string path, TimeSpan? @default = null, bool
/// <returns>A string containing the current configuration.</returns>
public override string ToString()
{
if (Root == null)
if (Value == null)
return "";

return Root.ToString();
return Value.ToString();
}

/// <summary>
Expand All @@ -438,21 +443,7 @@ public string ToString(bool includeFallback)
{
if (includeFallback == false)
return ToString();

Config current = this;

if (current.Fallback == null)
return current.ToString();

Config clone = Copy();

while (current.Fallback != null)
{
clone.Root.GetObject().Merge(current.Fallback.Root.GetObject());
current = current.Fallback;
}

return clone.ToString();
return Root.ToString();
}

/// <summary>
Expand All @@ -467,7 +458,10 @@ public virtual Config WithFallback(Config fallback)
throw new ArgumentException("Config can not have itself as fallback", nameof(fallback));
if (fallback == null)
return this;
var mergedRoot = Root.GetObject().MergeImmutable(fallback.Root.GetObject());
if (IsEmpty)
Aaronontheweb marked this conversation as resolved.
Show resolved Hide resolved
return fallback;

var mergedRoot = fallback.Root.GetObject().MergeImmutable(Root.GetObject());
var newRoot = new HoconValue();
newRoot.AppendValue(mergedRoot);
var mergedConfig = Copy(fallback);
Expand Down
21 changes: 18 additions & 3 deletions src/core/Akka/Configuration/Hocon/HoconObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ public string ToString(int indent)
var sb = new StringBuilder();
foreach (var kvp in Items)
{
if (kvp.Value.AdoptedFromFallback) continue;
Aaronontheweb marked this conversation as resolved.
Show resolved Hide resolved
string key = QuoteIfNeeded(kvp.Key);
sb.AppendFormat("{0}{1} : {2}\r\n", i, key, kvp.Value.ToString(indent));
}
Expand All @@ -186,6 +185,7 @@ public void Merge(HoconObject other)
{
var thisItems = Items;
var otherItems = other.Items;
var modified = new List<KeyValuePair<string, HoconValue>>();

foreach (var otherItem in otherItems)
{
Expand All @@ -195,14 +195,27 @@ public void Merge(HoconObject other)
{
//if both values are objects, merge them
if (thisItem.IsObject() && otherItem.Value.IsObject())
thisItem.GetObject().Merge(otherItem.Value.GetObject());
{
var newObject = thisItem.GetObject().MergeImmutable(otherItem.Value.GetObject());
var value = new HoconValue();
value.Values.Add(newObject);
modified.Add(new KeyValuePair<string, HoconValue>(otherItem.Key, value));
}
else
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, so if I'm reading this right, if both items are objects, they are to be merged, but how is the alternative supposed to work? what is otherItem's type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HOCON have 3 basic datatypes, Object, Array, and Literal.
Array and Literal will always override merged object fields, but when an Object met another Object, they got merged instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The full specification for HOCON can be read here: https://github.com/akkadotnet/hocon

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, I see it now

Copy link
Contributor

Choose a reason for hiding this comment

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

let's give it a shot then

modified.Add(new KeyValuePair<string, HoconValue>(otherItem.Key, otherItem.Value));
}
else
{
//other key was not present in this object, just copy it over
Items.Add(otherItem.Key, otherItem.Value);
modified.Add(new KeyValuePair<string, HoconValue>(otherItem.Key, otherItem.Value));
}
}

if (modified.Count == 0)
return;

foreach(var kvp in modified)
Items[kvp.Key] = kvp.Value;
}

/// <summary>
Expand All @@ -228,6 +241,8 @@ internal HoconObject MergeImmutable(HoconObject other)
mergedValue.AppendValue(mergedObject);
thisItems[otherItem.Key] = mergedValue;
}
else
thisItems[otherItem.Key] = otherItem.Value;
Copy link
Member

Choose a reason for hiding this comment

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

So for non-HoconObject types, we just ignored them on merge before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it causes a lot of missing object fields, I'm surprised that this bug went through the test suite

Copy link
Member

Choose a reason for hiding this comment

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

Is there a simple test case we can add for this to the HOCON test suite? Thinking about it mostly as a regression test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have rigorous testing suite in the standalone HOCON project, I think we used to have a limited HOCON test suite in Akka, they might be accidentally removed.

}
else
{
Expand Down