[BUG] Executing multiple Redis requests in parallel throws exception because GetProfilingSession() is not thread safe

[asliazas]

APM Agent version

1.25.3

Environment

Windows 10

.NET 8

Describe the bug

Executing multiple Redis requests in parallel sometimes throws exception because GetProfilingSession() is not thread safe. Exception I get:

System.ArgumentException: An item with the same key has already been added.
   at System.Runtime.CompilerServices.ConditionalWeakTable`2.Add(TKey key, TValue value)
   at Elastic.Apm.StackExchange.Redis.ElasticApmProfiler.GetProfilingSession()
   at StackExchange.Redis.RedisBase.ExecuteAsync[T](Message message, ResultProcessor`1 processor, ServerEndPoint server) in /_/src/StackExchange.Redis/RedisBase.cs:line 52
   at StackExchange.Redis.RedisDatabase.StringGetAsync(RedisKey key, CommandFlags flags) in /_/src/StackExchange.Redis/RedisDatabase.cs:line 3039

I see that GetProfilingSession() checks whether the key already exists and returns session, but in can be created after the check.

	var isSpan = realSpan != null;
	if (_executionSegmentSessions.TryGetValue(executionSegment, out var session))
		return session;

	_logger.Value.Trace()?.Log("Creating profiling session for {ExecutionSegment} {Id}",
	isSpan ? "span" : "transaction", executionSegment.Id);

	session = new ProfilingSession();

	_executionSegmentSessions.Add(executionSegment, session);

_executionSegmentSessions.Add then later throws exception if such key already exists. I think it should just return existing value instead.

        public void Add(TKey key, TValue value)
        {
            if (key is null)
            {
                ThrowHelper.ThrowArgumentNullException(ExceptionArgument.key);
            }

            lock (_lock)
            {
                int entryIndex = _container.FindEntry(key, out _);
                if (entryIndex != -1)
                {
                    ThrowHelper.ThrowArgumentException(ExceptionResource.Argument_AddingDuplicate);
                }

                CreateEntry(key, value);
            }
        }

To Reproduce

Create a List<Tasks> which executes a call to Redis and run all these tasks in parallel using await Task.WhenAll(tasks);

Expected behavior

Tasks should be executed successfully consistently

Actual behavior

Sometimes the following exception is thrown:

System.ArgumentException: An item with the same key has already been added.
   at System.Runtime.CompilerServices.ConditionalWeakTable`2.Add(TKey key, TValue value)
   at Elastic.Apm.StackExchange.Redis.ElasticApmProfiler.GetProfilingSession()
   at StackExchange.Redis.RedisBase.ExecuteAsync[T](Message message, ResultProcessor`1 processor, ServerEndPoint server) in /_/src/StackExchange.Redis/RedisBase.cs:line 52
   at StackExchange.Redis.RedisDatabase.StringGetAsync(RedisKey key, CommandFlags flags) in /_/src/StackExchange.Redis/RedisDatabase.cs:line 3039

[Mpdreamz]

Thanks for bringing this bug to our attention @asliazas!

I opened #2303 to address this.

[jdivy]

I just encountered this in some non-production testing and was surprised to find that an exception thrown in the profiling session would bubble all the way up to the multiplexer command call (in my case a call to .Subscribe()). I noticed you have a large try/catch + internal error log wrapping the 'EndProfilingSession' method and wondered if it's worth exploring a similar belt and suspenders approach on the 'GetProfilingSession' method for keeping future potential bugs from impacting redis calls?