[ 
https://issues.apache.org/jira/browse/TINKERPOP-1774?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16579445#comment-16579445
 ] 

ASF GitHub Bot commented on TINKERPOP-1774:
-------------------------------------------

Github user jorgebay commented on a diff in the pull request:

    https://github.com/apache/tinkerpop/pull/903#discussion_r209870927
  
    --- Diff: gremlin-dotnet/src/Gremlin.Net/Driver/ConnectionPool.cs ---
    @@ -33,91 +34,134 @@ internal class ConnectionPool : IDisposable
         {
             private readonly ConnectionFactory _connectionFactory;
             private readonly ConcurrentBag<Connection> _connections = new 
ConcurrentBag<Connection>();
    -        private readonly object _connectionsLock = new object();
    +        private readonly AutoResetEvent _newConnectionAvailable = new 
AutoResetEvent(false);
    +        private readonly int _minPoolSize;
    +        private readonly int _maxPoolSize;
    +        private readonly TimeSpan _waitForConnectionTimeout;
    +        private int _nrConnections;
     
    -        public ConnectionPool(ConnectionFactory connectionFactory)
    +        public ConnectionPool(ConnectionFactory connectionFactory, 
ConnectionPoolSettings settings)
             {
                 _connectionFactory = connectionFactory;
    +            _minPoolSize = settings.MinSize;
    +            _maxPoolSize = settings.MaxSize;
    +            _waitForConnectionTimeout = settings.WaitForConnectionTimeout;
    +            PopulatePoolAsync().WaitUnwrap();
             }
     
    -        public int NrConnections { get; private set; }
    +        public int NrConnections => Interlocked.CompareExchange(ref 
_nrConnections, 0, 0);
    +
    +        private async Task PopulatePoolAsync()
    +        {
    +            var connectionCreationTasks = new 
List<Task<Connection>>(_minPoolSize);
    +            for (var i = 0; i < _minPoolSize; i++)
    +            {
    +                connectionCreationTasks.Add(CreateNewConnectionAsync());
    +            }
    +
    +            var createdConnections = await 
Task.WhenAll(connectionCreationTasks).ConfigureAwait(false);
    +            foreach (var c in createdConnections)
    +            {
    +                _connections.Add(c);
    +            }
    +
    +            Interlocked.CompareExchange(ref _nrConnections, _minPoolSize, 
0);
    +        }
     
             public async Task<IConnection> GetAvailableConnectionAsync()
             {
    -            if (!TryGetConnectionFromPool(out var connection))
    -                connection = await 
CreateNewConnectionAsync().ConfigureAwait(false);
    +            if (TryGetConnectionFromPool(out var connection))
    +                return ProxiedConnection(connection);
    +            connection = await 
AddConnectionIfUnderMaximumAsync().ConfigureAwait(false) ?? WaitForConnection();
    +            return ProxiedConnection(connection);
    +        }
     
    -            return new ProxyConnection(connection, AddConnectionIfOpen);
    +        private IConnection ProxiedConnection(Connection connection)
    +        {
    +            return new ProxyConnection(connection, ReturnConnectionIfOpen);
             }
     
    -        private bool TryGetConnectionFromPool(out Connection connection)
    +        private void ReturnConnectionIfOpen(Connection connection)
    +        {
    +            if (!connection.IsOpen)
    +            {
    +                ConsiderUnavailable();
    +                DefinitelyDestroyConnection(connection);
    +                return;
    +            }
    +
    +            _connections.Add(connection);
    +            _newConnectionAvailable.Set();
    +        }
    +
    +        private async Task<Connection> AddConnectionIfUnderMaximumAsync()
             {
                 while (true)
                 {
    -                connection = null;
    -                lock (_connectionsLock)
    -                {
    -                    if (_connections.IsEmpty) return false;
    -                    _connections.TryTake(out connection);
    -                }
    +                var nrOpened = Interlocked.CompareExchange(ref 
_nrConnections, 0, 0);
    +                if (nrOpened >= _maxPoolSize) return null;
     
    -                if (connection.IsOpen) return true;
    -                connection.Dispose();
    +                if (Interlocked.CompareExchange(ref _nrConnections, 
nrOpened + 1, nrOpened) == nrOpened)
    +                    break;
                 }
    +
    +            return await CreateNewConnectionAsync().ConfigureAwait(false);
             }
     
             private async Task<Connection> CreateNewConnectionAsync()
             {
    -            NrConnections++;
                 var newConnection = _connectionFactory.CreateConnection();
                 await newConnection.ConnectAsync().ConfigureAwait(false);
                 return newConnection;
             }
     
    -        private void AddConnectionIfOpen(Connection connection)
    +        private Connection WaitForConnection()
             {
    -            if (!connection.IsOpen)
    +            var start = DateTimeOffset.Now;
    +            var remaining = _waitForConnectionTimeout;
    +            do
                 {
    -                ConsiderUnavailable();
    -                connection.Dispose();
    -                return;
    -            }
    -            AddConnection(connection);
    +                if (_newConnectionAvailable.WaitOne(remaining))
    --- End diff --
    
    This loop blocks the calling thread, which is a problem as we would be 
mixing async and blocking code.
    
    We should implemented in a way that it awaits for a connection to be 
available. Here are some good starting points (I'm not aware of a built-in 
mechanism in .NET): 
    - https://github.com/StephenCleary/AsyncEx/wiki/AsyncAutoResetEvent
    - 
https://blogs.msdn.microsoft.com/pfxteam/2012/02/11/building-async-coordination-primitives-part-2-asyncautoresetevent/


> Gremlin .NET: Support min and max sizes in Connection pool
> ----------------------------------------------------------
>
>                 Key: TINKERPOP-1774
>                 URL: https://issues.apache.org/jira/browse/TINKERPOP-1774
>             Project: TinkerPop
>          Issue Type: Improvement
>          Components: dotnet
>    Affects Versions: 3.2.7
>            Reporter: Jorge Bay
>            Assignee: Florian Hockmann
>            Priority: Minor
>
> Similar to the java connection pool, we should limit the maximum amount of 
> connections and start with a minimum number.
> It would also a good opportunity to remove the synchronous acquisitions of 
> {{lock}} in the pool implementation.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to