Update reconnect protocol to deal with half-open connections #276

fhcarter · 2021-08-09T23:14:17Z

Fixes #275 -- will require fix to Vantiq/vantiq-Issues#295 to work completely.

Adds reconnect secret to the connection requests. This secret allows a connector to usurp its old connection to Vantiq. This is required when the server fails to notice that the connection between the connector and the server has been unavailable. This is not uncommon in unstable network connection cases. When this happens, the connector's attempts to reconnect are rejected by the server because it (thinks it) already has a connection for the source in question.

This fix, in a parallel with the server side fix, allows the connector, using a reconnect secret to assert that it is the connection the server thinks it knows about. The server will, then, honor the connection & close the old, now defunct, session.

The fix uses parts of the connect extension message not previous used, so old servers will ignore it. And it is optional, so old clients talking to new servers will be fine.

This will allow _this_ client to reconnect to the same source, allowing it to recover from the _half open_ condition.

ghost · 2021-08-09T23:16:54Z

extjsdk/src/main/java/io/vantiq/extjsdk/ExtensionServiceMessage.java

@@ -38,6 +38,9 @@
    public static final String RESPONSE_ADDRESS_HEADER = "X-Reply-Address";
    public static final String PROPERTY_MESSAGE_HEADERS = "messageHeaders";

+    // Set in parameters to allow reconnect after network half-open situation
+    public static String RECONNECT_SECRET = "reconnectSecret";


[PMD Java] Do not use non-final non-private static fields (view)

Rule Ruleset Priority

MutableStaticState Design 3

References:

https://pmd.github.io/pmd-6.36.0/pmd_rules_java_design.html#mutablestaticstate

You can close this issue if no need to fix it. Learn more.

ghost · 2021-08-09T23:16:54Z

extjsdk/src/test/java/io/vantiq/extjsdk/RoundTripTestBase.java

@@ -29,6 +29,7 @@
    public static String testSourceName = null;
    public static String testTypeName = null;
    public static String testRuleName = null;
+    public static Boolean testRepeatedConnectsEnabled = null;


[PMD Java] Do not use non-final non-private static fields (view)

Rule Ruleset Priority

MutableStaticState Design 3

References:

https://pmd.github.io/pmd-6.36.0/pmd_rules_java_design.html#mutablestaticstate

You can close this issue if no need to fix it. Learn more.

ghost · 2021-08-09T23:16:54Z

extjsdk/src/main/java/io/vantiq/extjsdk/ExtensionServiceMessage.java

@@ -38,6 +38,9 @@
    public static final String RESPONSE_ADDRESS_HEADER = "X-Reply-Address";
    public static final String PROPERTY_MESSAGE_HEADERS = "messageHeaders";

+    // Set in parameters to allow reconnect after network half-open situation


[PMD Java] Too many fields (view)

Rule Ruleset Priority

TooManyFields Design 3

References:

https://pmd.github.io/pmd-6.36.0/pmd_rules_java_design.html#toomanyfields

You can close this issue if no need to fix it. Learn more.

ghost · 2021-08-09T23:16:54Z

extjsdk/src/main/java/io/vantiq/extjsdk/ExtensionWebSocketClient.java

+     * Unique identifier for this loader's class.  This is set on load and then left.  This
+     * shared secret allows instances of this client to perform reconnects to the server.
+     */
+    protected final static String clientReconnectSecret = UUID.randomUUID().toString();


[Checkstyle] 'static' modifier out of order with the JLS suggestions. (view)

Rule Severity

ModifierOrderCheck info

References:

https://checkstyle.org/config_modifier.html#ModifierOrder

You can close this issue if no need to fix it. Learn more.

ghost · 2021-08-09T23:16:54Z

extjsdk/src/main/java/io/vantiq/extjsdk/ExtensionServiceMessage.java

@@ -38,6 +38,9 @@
    public static final String RESPONSE_ADDRESS_HEADER = "X-Reply-Address";
    public static final String PROPERTY_MESSAGE_HEADERS = "messageHeaders";

+    // Set in parameters to allow reconnect after network half-open situation
+    public static String RECONNECT_SECRET = "reconnectSecret";


[Checkstyle] Name 'RECONNECT_SECRET' must match pattern '^[a-z][a-zA-Z0-9]*$'. (view)

Rule Severity

StaticVariableNameCheck info

References:

https://checkstyle.org/config_naming.html#StaticVariableName

You can close this issue if no need to fix it. Learn more.

fhcarter · 2021-08-09T23:14:42Z

extjsdk/build.gradle

+
+    if (rootProject.hasProperty("TestRepeatedConnects")) {
+        systemProperty "TestRepeatedConnects", rootProject.findProperty("TestRepeatedConnects") ?: "empty"
+    }


Temporary until we're working with 1.32 systems

fhcarter · 2021-08-09T23:16:10Z

extjsdk/src/main/java/io/vantiq/extjsdk/ExtensionWebSocketClient.java

+     * shared secret allows instances of this client to perform reconnects to the server.
+     */
+    protected final static String clientReconnectSecret = UUID.randomUUID().toString();
+


This is the actual secret. It is generated at class load/initialization time and allows this connector instance to reconnect at will. It will be the same for all sources to which this connector connects, but that's irrelevant -- we just want to prove ownership.

fhcarter · 2021-08-09T23:16:39Z

extjsdk/src/test/java/io/vantiq/extjsdk/RoundTripTestBase.java

@@ -40,6 +41,8 @@ public static void getProps() {
        testSourceName = System.getProperty("EntConTestSourceName", "testSourceName");
        testTypeName = System.getProperty("EntConTestTypeName", "testTypeName");
        testRuleName = System.getProperty("EntConTestRuleName", "testRuleName");
+        testRepeatedConnectsEnabled =
+                Boolean.valueOf(System.getProperty("TestRepeatedConnects", "false"));


Temporary -- we can remove when we're testing mostly against 1.32+ systems

We should probably add an issue reminding us to remove this once 1.32 is out.

fhcarter · 2021-08-09T23:16:56Z

extjsdk/src/test/java/io/vantiq/extjsdk/TestWithServer.java

+            assertTrue("Failed to open connection", client.isOpen());
+            assertTrue("Failed to authenticate", client.isAuthed());
+            assertTrue("Failed to connect to source (may fail if run against Vantiq version < 1.32)",
+                    client.isConnected());


but just in case, we'll alert...

ghost · 2021-08-09T23:42:04Z

extjsdk/src/test/resources/simulateUnstableNetwork.sh

+# This allows for manual testing for connector disconnection recovery.
+
+for ((i = 0; i < 100; i++)) do
+    echo Attempt $i;


[ShellCheck] Double quote to prevent globbing and word splitting. (view)

Rule Severity Code

SC2086 info 2086

References:

https://github.com/koalaman/shellcheck/wiki/SC2086

You can close this issue if no need to fix it. Learn more.

ghost · 2021-08-09T23:42:04Z

extjsdk/src/test/resources/simulateUnstableNetwork.sh

+# Shell script for Mac OS that will turn off then on wifi to monkey with network.
+# This allows for manual testing for connector disconnection recovery.
+
+for ((i = 0; i < 100; i++)) do


[ShellCheck] In POSIX sh, arithmetic for loops are undefined. (view)

Rule Severity Code

SC3005 warning 3005

References:

https://github.com/koalaman/shellcheck/wiki/SC3005

You can close this issue if no need to fix it. Learn more.

This should be ignored as per spellcheck directive atop file

ghost · 2021-08-09T23:42:04Z

extjsdk/src/test/resources/simulateUnstableNetwork.sh

+# Shell script for Mac OS that will turn off then on wifi to monkey with network.
+# This allows for manual testing for connector disconnection recovery.
+
+for ((i = 0; i < 100; i++)) do


[ShellCheck] In POSIX sh, ++ is undefined. (view)

Rule Severity Code

SC3018 warning 3018

References:

https://github.com/koalaman/shellcheck/wiki/SC3018

You can close this issue if no need to fix it. Learn more.

This should be ignored as per spellcheck directive atop file

fhcarter · 2021-08-09T23:45:59Z

Related Vantiq PR: Vantiq/ag2rs#8032

fhcarter · 2021-08-09T23:46:45Z

Jenkins run

OPCUA failures are linux/amz vs Mac OS.

fhcarter · 2021-08-09T23:56:48Z

@rblumer4, this may be related to the OPC errors. Looks like it all stems from some bug(s) in OpenJDK...

bcgit/bc-java#941 says it's fixed in their 301 release. It's not important to fix this now, just noting for the record.

namirfa

LGTM

Two minor comments, one about adding a new issue and one about a potential name change. Feel free to ignore.

namirfa · 2021-08-10T00:44:31Z

extjsdk/src/main/java/io/vantiq/extjsdk/ExtensionServiceMessage.java

@@ -38,6 +39,9 @@
    public static final String RESPONSE_ADDRESS_HEADER = "X-Reply-Address";
    public static final String PROPERTY_MESSAGE_HEADERS = "messageHeaders";

+    // Set in parameters to allow reconnect after network half-open situation
+    public static final String RECONNECT_SECRET = "reconnectSecret";


Should this be the "reconnect secret" or "connect secret"?

I think it's fine either way, but the secret would be used for both the first connection as well as any other reconnection afterwards.

I'd prefer to leave it as is. CONNECT_SECRET sounds like something that you need to connect (and, generally, you don't). It's also invisible to users of the SDK.

I'll think if there's a comment I can add that clarifies things.

Works for me!

namirfa · 2021-08-10T00:46:14Z

extjsdk/src/test/java/io/vantiq/extjsdk/RoundTripTestBase.java

@@ -40,6 +41,8 @@ public static void getProps() {
        testSourceName = System.getProperty("EntConTestSourceName", "testSourceName");
        testTypeName = System.getProperty("EntConTestTypeName", "testTypeName");
        testRuleName = System.getProperty("EntConTestRuleName", "testRuleName");
+        testRepeatedConnectsEnabled =
+                Boolean.valueOf(System.getProperty("TestRepeatedConnects", "false"));


We should probably add an issue reminding us to remove this once 1.32 is out.

fhcarter · 2021-08-10T00:51:02Z

Issue to remove stuff is #277

fhcarter · 2021-08-10T00:58:05Z

Jenkins re-run after all the changes...

(no changes)

namirfa · 2021-08-10T01:00:04Z

extjsdk/src/main/java/io/vantiq/extjsdk/ExtensionServiceMessage.java

+    // This is sent on any CONNECT_EXTENSION messages.  It is used when a connector
+    // "re-connects" to the server to verify that it's the same connector connecting in
+    // (otherwise, the reconnect is rejected).  So, while it's send on all CONNECT... calls,
+    // it's use is in only in the reconnect case (that is, usurping an existing connection).
+    // The name's a bit confusing, but CONNECT_SECRET sounds too much like something you need
+    // to connect (which, under normal circumstances, you don't).


Suggested change

// This is sent on any CONNECT_EXTENSION messages. It is used when a connector

// "re-connects" to the server to verify that it's the same connector connecting in

// (otherwise, the reconnect is rejected). So, while it's send on all CONNECT... calls,

// it's use is in only in the reconnect case (that is, usurping an existing connection).

// The name's a bit confusing, but CONNECT_SECRET sounds too much like something you need

// to connect (which, under normal circumstances, you don't).

// This is sent on any CONNECT_EXTENSION messages. It is used when a connector

// "re-connects" to the server to verify that it's the same connector connecting in

// (otherwise, the reconnect is rejected). So, while it's sent on all CONNECT... calls,

// its use is only in the reconnect case (that is, usurping an existing connection).

// The name's a bit confusing, but CONNECT_SECRET sounds too much like something you need

// to connect (which, under normal circumstances, you don't).

namirfa · 2021-08-10T01:00:52Z

Jenkins re-run after all the changes...

(no changes)

(wrong link)

Corrected link

fhcarter · 2021-08-10T18:09:22Z

Note: For this to fully work, this requires an update to the Vantiq server. This is anticipated in release 1.32.
And has been backported to 1.31.16

fhcarter added 3 commits August 9, 2021 14:27

Add reconnect secret constant

bba2226

Set reconnect secret for each connection.

788103b

This will allow _this_ client to reconnect to the same source, allowing it to recover from the _half open_ condition.

Add tests for reconnect secrets

df67ed9

fhcarter requested review from AlexBlumer and namirfa as code owners August 9, 2021 23:14

ghost reviewed Aug 9, 2021

View reviewed changes

fhcarter commented Aug 9, 2021

View reviewed changes

Add tool to simulate unstable network on Mac OS

51ac5ab

ghost reviewed Aug 9, 2021

View reviewed changes

fhcarter added 2 commits August 9, 2021 17:06

Fix SIDER issues

06586d7

More SIDER fixes/ignores

775cd84

namirfa approved these changes Aug 10, 2021

View reviewed changes

Add some explication around RECONNECT_SECRET

3771800

namirfa reviewed Aug 10, 2021

View reviewed changes

typos

327ccef

fhcarter merged commit 9a47e7f into master Aug 10, 2021

fhcarter deleted the fhc-updateReconnect-#275 branch August 10, 2021 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update reconnect protocol to deal with half-open connections #276

Update reconnect protocol to deal with half-open connections #276

fhcarter commented Aug 9, 2021

ghost Aug 9, 2021

ghost Aug 9, 2021

ghost Aug 9, 2021

ghost Aug 9, 2021

ghost Aug 9, 2021

fhcarter Aug 9, 2021

fhcarter Aug 9, 2021

fhcarter Aug 9, 2021

namirfa Aug 10, 2021

fhcarter Aug 9, 2021

ghost Aug 9, 2021

ghost Aug 9, 2021

fhcarter Aug 10, 2021

ghost Aug 9, 2021

fhcarter Aug 10, 2021

fhcarter commented Aug 9, 2021

fhcarter commented Aug 9, 2021 •

edited

Loading

fhcarter commented Aug 9, 2021 •

edited

Loading

namirfa left a comment

namirfa Aug 10, 2021

fhcarter Aug 10, 2021

namirfa Aug 10, 2021

namirfa Aug 10, 2021

fhcarter commented Aug 10, 2021

fhcarter commented Aug 10, 2021 •

edited

Loading

namirfa Aug 10, 2021

namirfa commented Aug 10, 2021 •

edited by fhcarter

Loading

fhcarter commented Aug 10, 2021 •

edited by rblumer4

Loading

Update reconnect protocol to deal with half-open connections #276

Update reconnect protocol to deal with half-open connections #276

Conversation

fhcarter commented Aug 9, 2021

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

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

fhcarter commented Aug 9, 2021

fhcarter commented Aug 9, 2021 • edited Loading

fhcarter commented Aug 9, 2021 • edited Loading

namirfa 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

fhcarter commented Aug 10, 2021

fhcarter commented Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

namirfa commented Aug 10, 2021 • edited by fhcarter Loading

fhcarter commented Aug 10, 2021 • edited by rblumer4 Loading

fhcarter commented Aug 9, 2021 •

edited

Loading

fhcarter commented Aug 9, 2021 •

edited

Loading

fhcarter commented Aug 10, 2021 •

edited

Loading

namirfa commented Aug 10, 2021 •

edited by fhcarter

Loading

fhcarter commented Aug 10, 2021 •

edited by rblumer4

Loading