Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Conversation

@halter73
Copy link
Member

@halter73 halter73 commented Jun 5, 2018

I haven't seen this cause any issues in practice, but the following sample shows why the way we're currently tracking connection lifetimes is at least theoretically bad. I know we also want to start tracking non-HTTP connections. This PR doesn't do that, but I think we should still take this change in the meantime.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

namespace ConnectionManagerRace
{
    public class Program
    {
        public static void Main(string[] args)
        {
            var manager = new ConnectionManager();
            Task.Run(() => StartConnections(100, manager));
            Thread.Sleep(500);
            manager.CloseAllConnectionsAsync().Wait();
        }

        public static void StartConnections(int count, ConnectionManager manager)
        {
            for (var i = 0; i < count; i++)
            {
                var conn = new Connection(manager);
                conn.StartRequestProcessing();
            }
        }
    }

    public class ConnectionManager
    {
        private readonly ConcurrentDictionary<long, Connection> _connections = new ConcurrentDictionary<long, Connection>();
        private long _lastId = 0;

        public void Add(Connection conn)
        {
            _connections.TryAdd(Interlocked.Increment(ref _lastId), conn);
        }

        public async Task CloseAllConnectionsAsync()
        {
            var closeTasks = new List<Task>();

            foreach (var pair in _connections)
            {
                closeTasks.Add(pair.Value.StopProcessingNextRequestAsync());
            }

            await Task.WhenAll(closeTasks);
        }
    }

    public class Connection
    {
        private readonly ConnectionManager _manager;
        private Task _lifetimeTask;

        public Connection(ConnectionManager manager)
        {
            _manager = manager;
        }

        public Task StartRequestProcessing()
        {
            return _lifetimeTask = ProcessRequestsAsync();
        }

        public async Task ProcessRequestsAsync()
        {
            _manager.Add(this);
            Thread.Sleep(1000);
            await Task.Delay(1000);
        }

        public Task StopProcessingNextRequestAsync()
        {
            return _lifetimeTask;
        }
    }
}

Which results in:

ConnectionManagerRace> dotnet run

Unhandled Exception: System.AggregateException: One or more errors occurred. (The tasks argument included a null value.
Parameter name: tasks) ---> System.ArgumentException: The tasks argument included a null value.
Parameter name: tasks
   at System.Threading.Tasks.Task.WhenAll(IEnumerable`1 tasks)
   at ConnectionManagerRace.ConnectionManager.CloseAllConnectionsAsync() in G:\dev\halter73\ConnectionManagerRace\Program.cs:line 48
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.Wait()
   at ConnectionManagerRace.Program.Main(String[] args) in G:\dev\halter73\ConnectionManagerRace\Program.cs:line 16

@halter73 halter73 requested a review from davidfowl June 5, 2018 00:12
@halter73 halter73 force-pushed the halter73/lifetime-tcs branch from e88220b to 0fcc60c Compare June 5, 2018 00:28
@halter73
Copy link
Member Author

@davidfowl ping

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

LGTM for now. We need to write up our design notes from the last discussion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants