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

Use freezable pattern for contract types instead of deep cloning #179

Closed
alnikola opened this issue May 20, 2020 · 3 comments · Fixed by #662
Closed

Use freezable pattern for contract types instead of deep cloning #179

alnikola opened this issue May 20, 2020 · 3 comments · Fixed by #662
Assignees
Labels
Type: Idea This issue is a high-level idea for discussion.

Comments

@alnikola
Copy link
Contributor

What should we add or change to make your life better?

Currently, we are making deep clones of all Abstractions.*.Contract types' instances (e.g. LoadBalancingOptions or QuotaOptions) before adding them to repositories. On top of that, some of them even have duplicates in RuntimeModel namespace with exactly the same contract, but different names (e.g. LoadBalancingOptions and BackendConfig.BackendLoadBalancingOptions). This is done to ensure safety and consistency of concurrent configuration reads in requests handling as well as to avoid accidental modifications.

However, it leads to code duplication and extra allocations in runtime. Thus, it's proposed to get rid of deep cloning and make all Abstractions.*.Contract types freezable which means they will be mutable during configuration initialization or refresh, but will prohibit any state changes after a Freeze() method is called. Each top-most configuration type (e.g. Backend and Route) will freeze it's state on a Freeze() call and ensure that all nested instances do the same (i.e. Backend.Freeze() must also call LoadBalancingOptions.Freeze()). Further, ProxyConfigLoader will call Freeze() on all top-most instances before adding them into repositories.

Why is this important to you?

It will eliminate code duplication, the reduce number of allocations and memory footprint.

@alnikola alnikola added the Type: Idea This issue is a high-level idea for discussion. label May 20, 2020
@Tratcher
Copy link
Member

@samsp-msft samsp-msft modified the milestones: 1.0.0-preview2, 1.0.0 May 27, 2020
@Tratcher
Copy link
Member

Tratcher commented Dec 8, 2020

Another proposal is to use C# 9 record types so the config objects are always immutable after creation.
https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-9#record-types

Known issues:

  • Redesign IProxyConfigFilter (not used by anyone yet?).
  • Some of the properties use mutable types like Cluster.Metadata is an IDictionary. These would need to become readonly collections. Would this complicate their creation?

@Tratcher Tratcher self-assigned this Dec 8, 2020
@Tratcher
Copy link
Member

Tratcher commented Dec 8, 2020

Note today we make some costly deep clones of many objects to avoid anyone mutating them while we're using them.

https://github.com/microsoft/reverse-proxy/pull/328/files#diff-62ef4e5f2fda56af2de14f76479908daa52ef2bb11732b1f08d39c5782085b39R63

Cluster IDeepCloneable<Cluster>.DeepClone()
{
return new Cluster
{
Id = Id,
LoadBalancing = LoadBalancing?.DeepClone(),
SessionAffinity = SessionAffinity?.DeepClone(),
HealthCheck = HealthCheck?.DeepClone(),
HttpClient = HttpClient?.DeepClone(),
HttpRequest = HttpRequest?.DeepClone(),
Destinations = Destinations.DeepClone(StringComparer.OrdinalIgnoreCase),
Metadata = Metadata?.DeepClone(StringComparer.OrdinalIgnoreCase),
};
}

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Idea This issue is a high-level idea for discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants