Do not purge inUse connections #260

zhenlineo · 2017-11-21T11:03:12Z

This PR consists of two parts:

When updating routing table, the pool of the servers that are removed from routing table should not be closed but be deactivated.
When a connection error happens towards a server, the pool of the server should not be purged but deactivated.

TODOs (Not in this PR but comes later):

While working on 2) we found that the DelegatedConnection.OnError should also provides a Async impl so that the connections could be closed synchronically. This will be left to some future work.
Chain async tasks if possible

Simplified test logic to not worry about connection validation if not needed Renamed `AvailableConnections` to `IdleConnections`

Added tests for verify deactiviation

zhenlineo · 2017-11-21T11:06:05Z

Neo4j.Driver/Neo4j.Driver/Internal/Routing/ClusterConnection.cs

+            throw error;
+        }
+
+        public async Task OnErrorAsync(Exception error)


@ali-ince This is the method that need to be plugged in DelegatedConnection

I think adding DelegatedConnection.OnErrorAsync makes sense. The only actual place where async job is done seems to be ClusterConnection though. This also brings another option of introducing IAsyncErrorHandler interface with OnErrorAsync and selectively calling it from within DelegatedConnection.TaskWithErrorHandling?

ali-ince

Looks good to me, with a couple of small comments.

ali-ince · 2017-11-21T11:33:35Z

Neo4j.Driver/Neo4j.Driver/Internal/ConnectionPool.cs

                    }

                    // Just dequeue any one connection and close it will ensure that all connections in the pool will finally be closed
-                    if (IsClosed && _availableConnections.TryTake(out connection))
+                    if (IsZombieOrClosed && _idleConnections.TryTake(out connection))


Shall we consider adding IsZombieOrClosed || check also to L438

ali-ince · 2017-11-21T11:34:06Z

Neo4j.Driver/Neo4j.Driver/Internal/ConnectionPool.cs

                    }

                    // Just dequeue any one connection and close it will ensure that all connections in the pool will finally be closed
-                    if (IsClosed && _availableConnections.TryTake(out connection))
+                    if (IsZombieOrClosed && _idleConnections.TryTake(out connection))


Shall we consider adding IsZombieOrClosed || check also to L478?

ali-ince · 2017-11-21T12:01:38Z

Neo4j.Driver/Neo4j.Driver/Internal/Routing/ClusterConnectionPool.cs

@@ -116,26 +115,62 @@ public void Add(IEnumerable<Uri> servers)
        {
            foreach (var uri in servers)
            {
-                Add(uri);
+                if (_pools.ContainsKey(uri))


It may be better and safer to make use of _pools.AddOrUpdate(uri, CreateNewConnectionPool, ActivateConnectionPool) where ActivateConnectionPool is a new method that just activates and returns the activated pool instance.

And also we can change GetOrAdd to TryAdd at L104.

ali-ince · 2017-11-21T13:29:19Z

Neo4j.Driver/Neo4j.Driver/Internal/Routing/LoadBalancer.cs

            }
            catch (ServiceUnavailableException e)
            {
-                OnConnectionError(uri, e);
+                await OnConnectionErrorAsync(uri, e).ConfigureAwait(false);
            }
            return null;


We probably did not notice the problem here before, but we should be returning Task.FromResult(null) instead of null.

Seems we do not need to cast as we have async keywords in the method declaration. And it will cast it to a Task automatically.

ali-ince · 2017-11-21T13:34:46Z

Neo4j.Driver/Neo4j.Driver/Internal/Routing/ClusterConnection.cs

+            throw error;
+        }
+
+        public async Task OnErrorAsync(Exception error)


I think adding DelegatedConnection.OnErrorAsync makes sense. The only actual place where async job is done seems to be ClusterConnection though. This also brings another option of introducing IAsyncErrorHandler interface with OnErrorAsync and selectively calling it from within DelegatedConnection.TaskWithErrorHandling?

ali-ince · 2017-11-21T13:57:19Z

I really do not know whether we should do or it would worth it but @lutovich introduced a fallback mechanism for Java Driver at LoadBalancer. What do you think?

…roperly

lutovich

@zhenlineo changes look good to me. Made couple small comments. Is it possible to add cluster IT for this new behaviour?

lutovich · 2017-11-22T08:24:12Z

Neo4j.Driver/Neo4j.Driver.Tests/ConnectionPoolTests.cs

+
+                var exception = Record.Exception(()=>pool.Acquire());
+
+                exception.Should().BeOfType<ObjectDisposedException>();


Maybe worth throwing some other exception when acquiring from zombie pool?
I thought ObjectDisposedException should only be thrown after Dispose() invocation.

lutovich · 2017-11-22T08:53:54Z

Neo4j.Driver/Neo4j.Driver/Internal/ConnectionPool.cs

-        private int _closedMarker = 0;
+        private int _poolStatus = Open;
+        private bool IsClosed => _poolStatus == Closed;
+        private bool IsZombieOrClosed => _poolStatus != Open;


Field _poolStatus is modified using Interlocked but is read using regular field accesses, is it ok?

Good point!

lutovich · 2017-11-22T08:56:41Z

Neo4j.Driver/Neo4j.Driver/Internal/Routing/ClusterConnectionPool.cs

        public IConnection Acquire(Uri uri)
        {
-            IConnectionPool pool;
-            if (!_pools.TryGetValue(uri, out pool))
+            if (!_pools.TryGetValue(uri, out var pool))


lutovich · 2017-11-22T09:03:06Z

Neo4j.Driver/Neo4j.Driver/Internal/Routing/ClusterConnectionPool.cs

+        public async Task UpdateAsync(IEnumerable<Uri> added, IEnumerable<Uri> removed)
+        {
+            await AddAsync(added).ConfigureAwait(false);
+            // TODO chain this part and use task.waitAll


Is this TODO still relevant?

This is the second TODO. Will do in another PR.

lutovich · 2017-11-22T09:10:26Z

Neo4j.Driver/Neo4j.Driver/Internal/ConnectionPool.cs

+    {
+        public const int Open = 0;
+        public const int Closed = 1;
+        public const int Zombie = 2;


Maybe rename "open" to "active" and "zombie" to "inactive"?

How about alive, zombie and totallyDead? :P I will renaming as you suggested.

Open -> Active Zombie -> Inactive

Zhen added 5 commits November 20, 2017 11:04

Delay purge connection that is current in use by sessions

eb21478

Added connection pool state tests and concurrent tests

b48f368

Simplified test logic to not worry about connection validation if not needed Renamed `AvailableConnections` to `IdleConnections`

Plug in routing table update routing table to not purge connections

63f961f

Remove purge on connection errors but deactiviate

0de98ba

Added tests for verify deactiviation

I miss-spelled deactivate

afa6493

zhenlineo commented Nov 21, 2017

View reviewed changes

zhenlineo requested review from ali-ince and lutovich November 21, 2017 11:06

ali-ince reviewed Nov 21, 2017

View reviewed changes

Made cluster pool to handle concurrent update (add, delete, modify) p…

cc42022

…roperly

lutovich reviewed Nov 22, 2017

View reviewed changes

Zhen added 3 commits November 22, 2017 13:02

Fixed reading is not atomic problem

eccd0b9

Renamed

1e78b6b

Open -> Active Zombie -> Inactive

Fix failing test

25dea0f

zhenlineo mentioned this pull request Nov 22, 2017

Plugged in OnErrorAsync #261

Merged

zhenlineo merged commit f0fbf34 into neo4j:1.5 Nov 23, 2017

zhenlineo deleted the 1.5-purge-improve branch November 23, 2017 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not purge inUse connections #260

Do not purge inUse connections #260

zhenlineo commented Nov 21, 2017 •

edited

Loading

zhenlineo Nov 21, 2017 •

edited

Loading

ali-ince Nov 21, 2017

ali-ince left a comment

ali-ince Nov 21, 2017

ali-ince Nov 21, 2017 •

edited

Loading

ali-ince Nov 21, 2017

ali-ince Nov 21, 2017

ali-ince Nov 21, 2017

zhenlineo Nov 21, 2017

ali-ince Nov 21, 2017

ali-ince Nov 21, 2017

ali-ince commented Nov 21, 2017

lutovich left a comment

lutovich Nov 22, 2017

lutovich Nov 22, 2017

zhenlineo Nov 22, 2017

lutovich Nov 22, 2017

lutovich Nov 22, 2017

zhenlineo Nov 22, 2017

lutovich Nov 22, 2017

zhenlineo Nov 22, 2017


		var exception = Record.Exception(()=>pool.Acquire());

		exception.Should().BeOfType<ObjectDisposedException>();

Do not purge inUse connections #260

Do not purge inUse connections #260

Conversation

zhenlineo commented Nov 21, 2017 • edited Loading

zhenlineo Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ali-ince left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ali-ince Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ali-ince commented Nov 21, 2017

lutovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenlineo commented Nov 21, 2017 •

edited

Loading

zhenlineo Nov 21, 2017 •

edited

Loading

ali-ince Nov 21, 2017 •

edited

Loading