Flagsmith / flagsmith-dotnet-client

.NET Standard Client for Flagsmith. Ship features with confidence using feature flags and remote config. Host yourself or use our hosted version at https://www.flagsmith.com/
https://www.flagsmith.com/
BSD 3-Clause "New" or "Revised" License
19 stars 12 forks source link

Exception: System.ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported #88

Closed ivastokic closed 2 months ago

ivastokic commented 5 months ago

Hi, We were getting this error in production environment on one of our pods:

Screenshot 2024-01-24 at 14 26 26

It started yesterday evening, and restarting of the pod stopped it. We followed your example how to register service, also didn't have any related changes recently. Stack pointing to this one:

public async Task TrackFeature(string featureName)
    {
      int num;
      this.AnalyticsData[featureName] = this.AnalyticsData.TryGetValue(featureName, out num) ? num + 1 : 1;
      if ((DateTime.Now - this._LastFlushed).Seconds <= this._FlushIntervalSeconds)
        return;
      await this.Flush();
    } 

Do you have any idea why this happened, could it be related to lack of thread safety?

We are using version 5.0.1

novakzaballa commented 5 months ago

Hi @ivastokic. Could you please provide us with more information? For example: Are you using .Net Core or .Net Framework? Which version? Also, from the info and context provided, it looks like a safe thread issue, however, it is hard to say the root cause or recommend a solution without more context. Can you generate a minimal project reproducing the issue and your current configuration?

ivastokic commented 5 months ago

Hi, it is .NET Core app, but is it also hard to reproduce it from out side. The change with introducing enableAnalytics ON, on registering FlagsmithClient was introduced in October 2023, and worked well so far, even when this occurred, it fail on only one of pods, fine on others. We will turn off analytics for now.

MaximTkachenko commented 5 months ago

It looks like you have a thread sefety issue inside AnalyticsProcessor:

So you read and modify this dictionary concurrently without any protection. I made a screenshot from the decompiled code: image

novakzaballa commented 4 months ago

Thank you @MaximTkachenko and sorry about the late reply! We will take a look into this issue soon. In the mean time if you prefer you can submit a PR and We'll get that merged for you ASAP.

novakzaballa commented 3 months ago

While I was not able to reproduce the issue with functional tests, or automatic tests using the SDK main functions, I was able to systematically replicate the issue accessing directly the TrackFeature method of the AnalycticsProcessor class and I put a fix in place in the above PR, which is ready for review now. I will update this thread as soon as we have the fix merged.