fix lost connection and reconnect automatically #83

Dmitry-Bryliuk · 2016-12-02T15:20:55Z

done:

logical connection/response reader understands it is in faulted state and accepts no more requests
physical connection and response reader are moved into logical connection
requests queue is a part of response reader and fully managed by it, including faulted state
new logical connection wrapper, which is checking connection availability before each request and dropping underlying logical connection and reconnects when no connection
ping tarantool directly (socket keepAlive options not available under linux) and drop connection on ping failed

q:

cyclical reconnect attempts? now only one attempt to reconnect per request (if not reconnecting in other request)

This change is

roman-kozachenko · 2016-12-06T07:42:18Z

src/tarantool.client/IResponseReader.cs

+
+        void SetFaultedState();
+
+        bool IsFaultedState { get; }


Can we make IsFaultedState writeable instead of SetFaultedState method?

roman-kozachenko · 2016-12-06T07:45:26Z

src/tarantool.client/LogicalConnection.cs

+
+        public bool IsConnected()
+        {
+            return !(_responseReader?.IsFaultedState ?? true) && (_physicalConnection?.IsConnected() ?? false);


Quite hard to read. Maybe better to split in several expressions\extract some variable?

Should be redesigned.

Initialization of ResponseReader and PhysicalConnection should be moved to constructor. Then all fields could be made readonly and there will be no reason for testing them for null.

roman-kozachenko · 2016-12-06T07:55:07Z

src/tarantool.client/ResponseReader.cs

-                _clientOptions.LogWriter?.WriteLine($"End reading from connection, read bytes count: {readBytesCount}");
-
-                if (ProcessReadBytes(readBytesCount))
+                if (readWork.IsCompleted)


What if readWork.IsCompleted == false? Can it be if reading is in progress?

aensidhe · 2016-12-06T13:02:36Z

src/tarantool.client/ILogicalConnection.cs

    {
+        Task Connect();
+
+        bool IsConnected();


There should be state, not only boolean flag.

aensidhe · 2016-12-06T13:04:16Z

src/tarantool.client/LogicalConnection.cs

+
+        public bool IsConnected()
+        {
+            return !(_responseReader?.IsFaultedState ?? true) && (_physicalConnection?.IsConnected() ?? false);


Should be redesigned.

aensidhe · 2016-12-06T13:05:18Z

src/tarantool.client/ILogicalConnection.cs

 namespace ProGaudi.Tarantool.Client
 {
-    public interface ILogicalConnection
+    using System;


Please, respect our coding guidelines: usings should be outside of namespace.

aensidhe · 2016-12-06T13:07:25Z

src/tarantool.client/LogicalConnection.cs

+
+        public bool IsConnected()
+        {
+            return !(_responseReader?.IsFaultedState ?? true) && (_physicalConnection?.IsConnected() ?? false);


Initialization of ResponseReader and PhysicalConnection should be moved to constructor. Then all fields could be made readonly and there will be no reason for testing them for null.

aensidhe · 2016-12-06T13:10:20Z

src/tarantool.client/LogicalConnectionWrapper.cs

+    using ProGaudi.Tarantool.Client.Model;
+    using ProGaudi.Tarantool.Client.Utils;
+
+    public class LogicalConnectionWrapper : ILogicalConnection


Why do we need this class? Maybe we should move all the code to Box?

prefix all calls in Box to check connection and reconnect, or implement something like ILogicalConnection (with ability to drop logical connection and reconnect) inside box and make calls to it

If it manages logical connections then this is a manager, not a wrapper.

aensidhe · 2016-12-06T13:11:41Z

src/tarantool.client/NetworkStreamPhysicalConnection.cs

        {
            _disposed = true;
-            _stream?.Dispose();
+            Interlocked.Exchange(ref _stream, null)?.Dispose();


Don't replace stream with null. This is bad design decision.

aensidhe · 2016-12-06T13:14:55Z

src/tarantool.client/ResponseReader.cs

+        {
+            lock (this)
+            {
+                if (_pendingRequests == null)


Why did we introduced lock there?

roman-kozachenko · 2016-12-08T11:14:37Z

src/tarantool.client/LogicalConnection.cs

+        {
+            if (_disposed)
+            {
+                return false;


Or we should throw exception?

then we need catch exceptions here instead of check: https://github.com/progaudi/tarantool-csharp/pull/83/files/4be0d2ca63c07b90d6c7d9449e3da7ebb0624b65#diff-8b31d61323fab83274f6898980abbf6bR59

roman-kozachenko · 2016-12-08T11:18:44Z

src/tarantool.client/ResponseReader.cs

-                _clientOptions.LogWriter?.WriteLine($"End reading from connection, read bytes count: {readBytesCount}");
-
-                if (ProcessReadBytes(readBytesCount))
+                if (readWork.Status == TaskStatus.RanToCompletion)


What if Status == Ready?

https://msdn.microsoft.com/en-us/library/system.threading.tasks.taskstatus(v=vs.110).aspx

aensidhe

A lot of locking introduced without clear reason. Necessary lock was removed.

aensidhe · 2016-12-15T13:56:04Z

src/tarantool.client/LogicalConnection.cs

+                return false;
+            }
+
+            if (!_responseReader.IsConnected() || !_physicalConnection.IsConnected())


IsConnected should not change state.

aensidhe · 2016-12-15T13:57:49Z

src/tarantool.client/LogicalConnectionWrapper.cs

+    using ProGaudi.Tarantool.Client.Model;
+    using ProGaudi.Tarantool.Client.Utils;
+
+    public class LogicalConnectionWrapper : ILogicalConnection


If it manages logical connections then this is a manager, not a wrapper.

aensidhe · 2016-12-15T14:04:18Z

src/tarantool.client/LogicalConnection.cs

            long headerLength;
            var headerBuffer = CreateAndSerializeBuffer(request, requestId, bodyBuffer, out headerLength);

-            lock (_physicalConnection)


This lock is necessary. Otherwise you'll end up with two threads mixing requests and broken pipeline

aensidhe · 2016-12-15T14:05:07Z

src/tarantool.client/NetworkStreamPhysicalConnection.cs

        public void Dispose()
        {
-            _disposed = true;
-            _stream?.Dispose();


Why do we need this lock?

aensidhe · 2016-12-15T14:05:30Z

src/tarantool.client/NetworkStreamPhysicalConnection.cs

+                _disposed = true;
+
+                _stream?.Dispose();
+                _socket?.Dispose();


Socket is already disposed. Look to constructor.

how it may be already disposed when we call dispose manually when disconnect detected?

Stream owns socket and will dispose it, because we pass True to stream parameter.

aensidhe · 2016-12-15T14:05:52Z

src/tarantool.client/ResponseReader.cs


+        public Task<MemoryStream> GetResponseTask(RequestId requestId)
+        {
+            lock (this)


Why do we need this lock?

avoid concurrent access to _pendingRequests, while it perhaps may be destroying and should not accept requests any more

Better make _pendingRequests not readonly and set it to null in Dispose via InterlockedExchange.

Then you can copy it here via Interlocked.Exchange to local variable and check for null-ness.

Then we can rid of _disposed flag or made it calculatable.

still minor posibility of race condition without explicit lock

…into fix/reconnect

aensidhe

Good work, we discussed changes.

aensidhe · 2016-12-23T09:52:36Z

src/tarantool.client/LogicalConnection.cs

+                    _physicalConnectionLock.ExitWriteLock();
+                }
+            }
+            catch (Exception ex)


Merge this into one try-catch-finally, please.

aensidhe · 2016-12-23T09:54:00Z

src/tarantool.client/LogicalConnectionManager.cs

+
+            _clientOptions.LogWriter?.WriteLine($"{nameof(LogicalConnectionManager)}: Connected...");
+
+            if (_clientOptions.ConnectionOptions.PingCheckInterval > 0)


Can we treat 0 as disabled ping?

aensidhe · 2016-12-23T10:43:55Z

src/tarantool.client/LogicalConnectionManager.cs

+
+        public async Task Connect()
+        {
+            _clientOptions.LogWriter?.WriteLine($"{nameof(LogicalConnectionManager)}: Connecting...");


We discussed it in person.

rewrite it to ReadWriteLockSlim.

What we miss in our discussion:

cleanup of old connection and old timer.

cleanup of _droppableLogicalConnection is in Connect or in LogicalConnectionManager.Dispose
cleanup of timer is in LogicalConnectionManager.Dispose

using ReadWriteLock, not Slim - the only wait point is Connect and it is long, no reason for spin wait

aensidhe · 2016-12-23T10:45:38Z

src/tarantool.client/ResponseReader.cs

+                TaskCompletionSource<MemoryStream> request;
+                if (_pendingRequests.TryGetValue(requestId, out request))
+                {
+                    _pendingRequests.Remove(requestId);


Remove does not throw exceptions if key is missing. We can remove check for TryGetValue.

with the check we avoid redundant call when value is missing. minor performance gain.

aensidhe · 2016-12-23T10:46:59Z

src/tarantool.client/ResponseReader.cs

+                _clientOptions.LogWriter?.WriteLine($"Connection read failed: {readWork.Exception}");
+                Dispose();
            }
            else


Please, reduce nesting.

aensidhe · 2016-12-27T13:00:07Z

Reviewed 13 of 13 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

src/tarantool.client/LogicalConnectionManager.cs, line 42 at r1 (raw file):

            _clientOptions = options;

            if (_clientOptions.ConnectionOptions.PingCheckInterval >= 0)

If zero is bad value for PingCheckInterval - incorporate validation logic into setter of it.

src/tarantool.client/LogicalConnectionManager.cs, line 80 at r1 (raw file):

                _connected.Reset();

                _onceConnected = true;

Why do we need this flag? It is not resetted to false ever.

src/tarantool.client/LogicalConnectionManager.cs, line 114 at r1 (raw file):

                }

                Task.WaitAny(SendRequestWithEmptyResponse(_pingRequest));

Why do we need to wait on it at all?

Comments from Reviewable

roman-kozachenko · 2016-12-27T14:53:24Z

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

src/tarantool.client/LogicalConnection.cs, line 157 at r1 (raw file):

            try
            {
                _physicalConnectionLock.EnterWriteLock();

How about send headerBuffer and bodyBuffer as 1 buffer (concat arrays befor sent?) and remove lock?

src/tarantool.client/LogicalConnectionManager.cs, line 42 at r1 (raw file):

Previously, aensidhe (Anatoly Popov) wrote…

If zero is bad value for PingCheckInterval - incorporate validation logic into setter of it.

As i understand - 0 is good value for PingCheckInterval. If it is zero, ping packets will not be sent.
But I agree, negative values can be validated in setter.

Comments from Reviewable

Dmitry-Bryliuk added 8 commits December 2, 2016 18:01

fix reconnect - introduce faulted state

d384023

phys connection and resp reader into logical conn

497f446

requests queue and faulted state inside resp reader

8252cf0

more distributed arcvitecture

a6efd0b

check resp reader fault state for connection alive

3e9ae2b

more log records

2ba2d78

some fixes

5ad8b6b

some fixes

e98e31f

roman-kozachenko reviewed Dec 6, 2016

View reviewed changes

aensidhe requested changes Dec 6, 2016

View reviewed changes

Dmitry-Bryliuk added 2 commits December 8, 2016 13:55

disposed/state redesign

a2a8b42

task status check fixed

4be0d2c

roman-kozachenko approved these changes Dec 8, 2016

View reviewed changes

aensidhe requested changes Dec 15, 2016

View reviewed changes

Dmitry-Bryliuk added 16 commits December 16, 2016 14:44

requested fixes

5c398f7

ping tarantool instead of socket ping, tbd

2df6fa1

ping tarantool, done

d64e990

fix reconnect - introduce faulted state

e2f4d32

phys connection and resp reader into logical conn

05f7d20

requests queue and faulted state inside resp reader

e3d9fa7

more distributed arcvitecture

8593122

check resp reader fault state for connection alive

a2c4c9a

more log records

8651c7e

some fixes

f919ed5

some fixes

1818693

disposed/state redesign

e327efb

task status check fixed

92554bd

requested fixes

09901fe

ping tarantool instead of socket ping, tbd

20cac5c

ping tarantool, done

1e10433

Dmitry-Bryliuk force-pushed the fix/reconnect branch from d64e990 to 1e10433 Compare December 20, 2016 15:20

Dmitry-Bryliuk added 5 commits December 20, 2016 18:23

Merge branch 'fix/reconnect' of github.com:progaudi/tarantool-csharp …

6ea4b2a

…into fix/reconnect

cosmetic fix

0497cfa

slim locks and non-concurrent dict for requests

968beb8

ping without timeout, alive interval for ping

9cd0659

ping check interval in ConnectionOptions

81d7857

aensidhe reviewed Dec 23, 2016

View reviewed changes

Dmitry-Bryliuk added 3 commits December 23, 2016 16:06

minor fixes

6e40a4d

not working version with ReadWriteLock, tbd

2159793

always reconnect with events

ddd59f4

remove unnecessary flag, use async call for ping

0750eee

aensidhe approved these changes Dec 28, 2016

View reviewed changes

aensidhe merged commit 9132a64 into master Dec 28, 2016

aensidhe deleted the fix/reconnect branch December 28, 2016 12:34


		_clientOptions.LogWriter?.WriteLine($"{nameof(LogicalConnectionManager)}: Connected...");

		if (_clientOptions.ConnectionOptions.PingCheckInterval > 0)

fix lost connection and reconnect automatically #83

fix lost connection and reconnect automatically #83

Uh oh!

Conversation

Dmitry-Bryliuk commented Dec 2, 2016 • edited by roman-kozachenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aensidhe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aensidhe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Dmitry-Bryliuk commented Dec 2, 2016 •

edited by roman-kozachenko

Loading