Concurrent collections in Dotnet are not as safe as they seem

[LukeButters]

Background

Concurrent dictionary appears to be surprising in that Func passed to it are not executed in what one would consider to be "thread safe", so resort to manual locking.

ConcurrentBag does interesting things with thread locals, which has lead to strange memory leaks:

• Fix memory leak due to use of ConcurrentBag autofac/Autofac#1257

so lets just not use it.

For background read: https://octopusdeploy.slack.com/archives/CMESRJ1C3/p1703022896511849

This change is not because we have been seeing issues, but is done preemptively to prevent issues.

The only non test change is the one done to the Redis PRQ.

How to review this PR

Quality ✔️

Pre-requisites

• I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
• I have considered informing or consulting the right people, according to the ownership map.
• I have considered appropriate testing for my change.

[Copilot]

Pull Request Overview

Replaces concurrent collections with manually locked alternatives to avoid thread safety issues in .NET. The PR addresses potential memory leaks and unexpected behavior with ConcurrentBag and ConcurrentDictionary by using explicit locking patterns instead.

• Replaced ConcurrentBag with List and manual locking in CancelOnDisposeCancellationToken
• Replaced ConcurrentDictionary operations with manual locking in CallCountingHalibutRedisTransport
• Updated documentation to reflect thread safety changes

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
CancelOnDisposeCancellationToken.cs	Replaced ConcurrentBag with List and added lock for thread safety
CallCountingHalibutRedisTransport.cs	Added manual locking around ConcurrentDictionary operations

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

The enumeration of tasks for Task.WhenAll is not thread-safe. This operation should be performed within a lock to ensure the collection doesn't change during enumeration.

Suggested change

	if (tasks.Count > 0)
	{
	await Task.WhenAll(tasks.Select(t => Try.IgnoringError(() => t)));
	}
	List<Task> tasksToAwait;
	lock (tasks)
	{
	tasksToAwait = tasks.Count > 0 ? new List<Task>(tasks) : null;
	}
	if (tasksToAwait != null)
	{
	await Task.WhenAll(tasksToAwait.Select(t => Try.IgnoringError(() => t)));
	}

[LukeButters]

The doco suggests not to call dispose concurrently with what can modify the list.

[david-staniec-octopus]

Is there a reason not to make the locking changes when reading from the collection here to prevent thread safety concerns given the move away from a ConcurrentBag?

[LukeButters]

We are trying to avoid extra work in the common case which is when we don't have anything to wait on.

This class has surprisingly shown up in the profiler, so performance seems to be a concern :(

I am not concerned about attempting to support unsupported use cases where someone is adding more tasks to wait for on an object they are currently disposing.

[david-staniec-octopus]

Discussed tradeoff here:

• The case I've highlighted could occur if we're trying to access the tasks when we're already disposing, which would still create unpredictable behaviour.
• The performance concerns around introducing additional locking in CancelOnDisposeCancellationToken means this is not something we'd like to introduce unless there's a strong requirement here.

Will approve this PR given the conscious trade off around performance.

[david-staniec-octopus]

Approving as per previous comment.