Skip to content

Commit

Permalink
YamlDotNot no longer appears to be thread-safe
Browse files Browse the repository at this point in the history
  • Loading branch information
nathanwoctopusdeploy committed Mar 5, 2024
1 parent f78a516 commit e17740b
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 7 deletions.
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() =>

Check warning on line 15 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 15 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 15 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 15 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 15 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 15 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 15 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build

Check warning on line 15 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build

Check warning on line 15 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build

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

//private static IDeserializer GetStrictDeserializer() =>

Check warning on line 24 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 24 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 24 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 24 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 24 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 24 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 24 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build

Check warning on line 24 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build

Check warning on line 24 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build

// GetCommonDeserializerBuilder()
// .WithDuplicateKeyChecking()
// .Build();

//private static IDeserializer GetDeserializer() =>

Check warning on line 29 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 29 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 29 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 29 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 29 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build

Check warning on line 29 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build

// GetCommonDeserializerBuilder()
// .IgnoreUnmatchedProperties()
// .Build();

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

Check warning on line 34 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 34 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / e2e

Check warning on line 34 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 34 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Check warning on line 34 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build

Check warning on line 34 in src/KubernetesClient/KubernetesYaml.cs

View workflow job for this annotation

GitHub Actions / MSBuild build


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

0 comments on commit e17740b

Please sign in to comment.