Skip to content

YamlDotNot is not thread-safe #1536

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

Closed
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
2 changes: 2 additions & 0 deletions src/KubernetesClient/KubernetesClient.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
<PackageReference Include="IdentityModel.OidcClient" Version="5.2.1" />
<PackageReference Include="Fractions" Version="7.3.0" />
<PackageReference Include="YamlDotNet" Version="15.1.0" />
<!--Same issue with the latest version of YamlDotNet-->
<!--<PackageReference Include="YamlDotNet" Version="15.1.2" />-->
</ItemGroup>

<ItemGroup>
Expand Down
29 changes: 25 additions & 4 deletions src/KubernetesClient/KubernetesYaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,27 @@ namespace k8s
/// </summary>
public static class KubernetesYaml
{
//private static DeserializerBuilder GetCommonDeserializerBuilder() =>
Copy link
Contributor Author

@nathanwoctopusdeploy nathanwoctopusdeploy Mar 5, 2024

Choose a reason for hiding this comment

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

Changing YamlDotNet Deserializer to no longer be a static makes the test pass

// new DeserializerBuilder()
// .WithNamingConvention(CamelCaseNamingConvention.Instance)
// .WithTypeConverter(new IntOrStringYamlConverter())
// .WithTypeConverter(new ByteArrayStringYamlConverter())
// .WithTypeConverter(new ResourceQuantityYamlConverter())
// .WithAttemptingUnquotedStringTypeDeserialization()
// .WithOverridesFromJsonPropertyAttributes();

//private static IDeserializer GetStrictDeserializer() =>
// GetCommonDeserializerBuilder()
// .WithDuplicateKeyChecking()
// .Build();

//private static IDeserializer GetDeserializer() =>
// GetCommonDeserializerBuilder()
// .IgnoreUnmatchedProperties()
// .Build();

//private static IDeserializer GetDeserializer(bool strict) => strict ? GetStrictDeserializer() : GetDeserializer();

private static DeserializerBuilder CommonDeserializerBuilder =>
new DeserializerBuilder()
.WithNamingConvention(CamelCaseNamingConvention.Instance)
Expand All @@ -23,12 +44,12 @@ public static class KubernetesYaml

private static readonly IDeserializer StrictDeserializer =
CommonDeserializerBuilder
.WithDuplicateKeyChecking()
.Build();
.WithDuplicateKeyChecking()
.Build();
private static readonly IDeserializer Deserializer =
CommonDeserializerBuilder
.IgnoreUnmatchedProperties()
.Build();
.IgnoreUnmatchedProperties()
.Build();
private static IDeserializer GetDeserializer(bool strict) => strict ? StrictDeserializer : Deserializer;

private static readonly IValueSerializer Serializer =
Expand Down
67 changes: 64 additions & 3 deletions tests/KubernetesClient.Tests/KubernetesClientConfigurationTests.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
using k8s.Authentication;
using k8s.Exceptions;
using k8s.KubeConfigModels;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.IO;
using System.IO.Abstractions;
Expand All @@ -10,6 +8,10 @@
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
using FluentAssertions;
using k8s.Authentication;
using k8s.Exceptions;
using k8s.KubeConfigModels;
using Xunit;

namespace k8s.Tests
Expand Down Expand Up @@ -638,6 +640,65 @@ public void ContextPreferencesExtensionsMergeWithDuplicates()
Assert.Single(cfg.Preferences);
}

[Fact]
public void LoadKubeConfigShouldBeThreadSafe()
{
// This is not guaranteed to fail but it usual throws the follow exception
// - System.InvalidOperationException: Operations that change non-concurrent collections must have exclusive access.
// A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
// at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
// at System.Collections.Generic.Dictionary`2.set_Item(TKey key, TValue value)
// at YamlDotNet.Serialization.ObjectFactories.DefaultObjectFactory.GetStateMethods(Type attributeType, Type valueType)
// at YamlDotNet.Serialization.ObjectFactories.DefaultObjectFactory.ExecuteState(Type attributeType, Object value)
// at YamlDotNet.Serialization.ObjectFactories.DefaultObjectFactory.ExecuteOnDeserializing(Object value)
// at YamlDotNet.Serialization.NodeDeserializers.ObjectNodeDeserializer.Deserialize(IParser parser, Type expectedType, Func`3 nestedObjectDeserializer, Object& value)
// at YamlDotNet.Serialization.ValueDeserializers.NodeValueDeserializer.DeserializeValue(IParser parser, Type expectedType, SerializerState state, IValueDeserializer nestedObjectDeserializer)}.

// YamlDotNet Deserializer does not seems to be thread safe, changing KubernetesYaml to not use a static serializer makes this test pass.

var exceptions = new ConcurrentStack<Exception>();

// Run it many times for a better failure rate
Run(exceptions);
Run(exceptions);
Run(exceptions);
Run(exceptions);
Run(exceptions);

exceptions.Should().HaveCount(0, "No exceptions should have been recorded");

static void Run(ConcurrentStack<Exception> exceptions)
{
var threadCount = 100;
var threads = new List<Thread>();
var control = new SemaphoreSlim(0, threadCount);

for (var i = 0; i < threadCount; i++)
{
threads.Add(new Thread(LoadKubeConfig));
}

threads.ForEach(t => t.Start());
control.Release(threadCount);
threads.ForEach(t => t.Join());

void LoadKubeConfig()
{
control.Wait();

try
{
var fileInfo = new FileInfo(Path.GetFullPath("assets/kubeconfig.yml"));
KubernetesClientConfiguration.LoadKubeConfig(new [] { fileInfo });
}
catch (Exception e)
{
exceptions.Push(e.InnerException ?? e);
}
}
}
}

/// <summary>
/// Ensures Kube config file can be loaded from within a non-default <see cref="SynchronizationContext"/>.
/// The use of <see cref="UIFactAttribute"/> ensures the test is run from within a UI-like <see cref="SynchronizationContext"/>.
Expand Down