Fixing the way Selector handles SSL connection handshakes #310

nsivabalan · 2016-05-16T16:50:02Z

Currently, for connections over SSL, the ambry selector returns an established connection after a poll even before the handshake is completed. This is not very useful as the caller will then have to repeatedly check outside of the selector poll on whether the handshake is complete, which is not ideal.

This patch fixes this issue.
The selector will only return a connection after the connection is established and the handshake is complete. If the handshake does not complete for any reason, then it should be treated as a connection establishment failure. This way the callers of the selector can be agnostic to SSL, and can interact with the selector in a completely non-blocking way.

Coverage:
Selector 100% (1/ 1) 96.4% (27/ 28) 84.5% (191/ 226)
All the new lines are covered

SLA: 10 mins
Reviewers: Priyesh, Ming

Addresses issue #297

xiahome · 2016-05-16T18:31:20Z

ambry-network/src/main/java/com.github.ambry.network/Selector.java

+        if (sslTransmission != null && sslTransmission.ready()) {
+          connected.add(sslTransmission.getConnectionId());
+          this.metrics.selectorConnectionCreated.inc();
+          Long handshakeStartTime = sslHandshakeTimer.remove(sslTransmission.getConnectionId());


A connectionId will only be removed here?

I will update the patch with a minor fix. Someone can close it explicitly too by calling close(key)

pnarayanan · 2016-05-16T21:48:44Z

ambry-network/src/main/java/com.github.ambry.network/Selector.java

@@ -72,6 +72,8 @@
  private final List<NetworkReceive> completedReceives;
  private final List<String> disconnected;
  private final List<String> connected;
+  private final List<SSLTransmission> pendingSslHandshakes;
+  private final Map<String, Long> sslHandshakeTimer;


Why not avoid the map completely? Just use a list of objects of a private class:

private class PendingHandshakeTransmission { SSLTransmission transmission; long pendingSinceMs; }

pnarayanan · 2016-05-17T00:17:38Z

Done with a pass. Let me know when you update the PR.

xiahome · 2016-05-17T04:01:58Z

ambry-network/src/main/java/com.github.ambry.network/Selector.java

@@ -455,6 +481,9 @@ private void close(SelectionKey key) {
      activeConnections.set(this.keyMap.size());
      try {
        transmission.close();
+        if (pendingHandshakeTransmissions.get(transmission.getConnectionId()) != null) {
+          pendingHandshakeTransmissions.remove(transmission.getConnectionId());
+        }
      } catch (IOException e) {


If a key can be closed in a different thread, would this part be thread safe?

Not related to this PR.
Transmission.close() will NOT throw an IOException if taking a look at its implementation in both PlainTextTransmission and SSLTransmission though the abstract method defines so.

yes, looks like we might have an issue. even keyMap and activeConnections looks to be affected. Lets discuss in person.

From the documentation it looks like the Selector isn't meant to be thread safe. The documentation of the class on line 64 says so. There are a lot of parts in this class that work on the fact that this class was not meant to be thread safe.

Also, I cannot think of why we would want to use this Selector across threads.

selector should never be used across threads

Sorry, my bad. I thought the processNewResponses() in SocketServer() was a daemon thread. Hence, the close(SelectionKey) could be called simultaneously when selector poll() is happening. Looks like its called sequentially and selector.poll() follows it. So, I don't think we have a problem of multiple threads accessing it.

xiahome · 2016-05-17T04:03:23Z

Left a few more minor comments. The rest LGTM.

xiahome · 2016-05-17T17:30:18Z

LGTM.

vgkholla · 2016-05-17T23:06:56Z

ambry-network/src/main/java/com.github.ambry.network/Selector.java

      this.metrics.selectorIORate.inc();
    }
    long endIo = time.milliseconds();
    this.metrics.selectorIOTime.update(endIo - endSelect);
  }

  /**
+   * Add those Ssl connections to connected list on handshake completion


think doc can be improved. What are "those"?

vgkholla · 2016-05-20T22:31:07Z

ambry-network/src/main/java/com.github.ambry.network/NetworkMetrics.java

+   * @param numConnectionsPendingHandhsake Count of connections that are awaiting handshake completion
+   */
+  public void initializeSelectorMetricsIfRequired(final AtomicLong activeConnections,
+      final AtomicLong numConnectionsPendingHandhsake) {


pnarayanan · 2016-05-20T22:32:32Z

@nsivabalan I don't think you have updated the patch.

vgkholla · 2016-05-20T22:32:44Z

ambry-network/src/main/java/com.github.ambry.network/NetworkMetrics.java

      @Override
      public Long getValue() {
        return activeConnections.get();
      }
    };
+    this.numConnectionsPendingHandhsake = new Gauge<Long>() {


looks like you named the other arg as activeConnections so that the this is avoided. Could you do the same here? (or name the other also as numActiveConnections). This is minor but I would like some consistency.

vgkholla · 2016-05-20T22:33:52Z

@pnarayanan just told me that this requires some more work from @nsivabalan. Holding off reviewing until that is resolved.

nsivabalan · 2016-05-21T03:09:24Z

@pnarayanan : can you tell me what are the comments yet to be addressed?

pnarayanan · 2016-05-21T03:35:09Z

"... I have removed it. Updated the patch" - what have you removed?

xiahome · 2016-05-21T05:17:22Z

ambry-network/src/main/java/com.github.ambry.network/Selector.java

@@ -72,10 +72,12 @@
  private final List<NetworkReceive> completedReceives;
  private final List<String> disconnected;
  private final List<String> connected;
+  private final List<String> connectionPendingHandshakes;


Should be connectionsPendingHandshake.

xiahome · 2016-05-21T05:19:29Z

@pnarayanan I think @nsivabalan meant the duplicate metric for ssl handshaking time is removed.

nsivabalan · 2016-05-21T16:05:43Z

@pnarayanan yes, I meant the same. That I have removed the duplicate metric

nsivabalan · 2016-05-21T16:06:07Z

@xiahome @pnarayanan @vgkholla : I have updated the patch.

pnarayanan · 2016-05-21T17:20:27Z

ambry-network/src/main/java/com.github.ambry.network/NetworkMetrics.java

@@ -44,7 +44,8 @@
  public final Counter selectorKeyOperationErrorCount;
  public final Counter selectorCloseKeyErrorCount;
  public final Counter selectorCloseSocketErrorCount;
-  public Gauge<Long> selectorActiveConnections;
+  public Gauge<Long> numActiveConnections;
+  public Gauge<Long> numConnectionsPendingHandshake;


This should not be a selector metric as this is not "general". This should be moved below under SSL Metrics and SSLTransmission should be the one updating it.

That said, I would suggest getting rid of this metric completely. This is a Gauge and number of connections pending handshake at a particular instant is not useful as these will be very low in number and they would either error out or get established. Gauge would be sort of useful for things that stay for a while like numActiveConnections.

This is just to track the pool (pending connections) size. This shouldn't be linearly growing. I expect the number to be stable as each connections take 5 to 6 poll() calls to complete its handshake. If the pending connections count is growing (which is not feasible to track if not for this metric), for some reason, we are doing something wrong. But that being said, I am not very pertinent in having this metric. @vgkholla @xiahome whats your take on this.

pnarayanan · 2016-05-21T17:28:43Z

Once again, selector should not refer to handshakes. Handshakes are internal to SSLTransmission. As far as the Selector is concerned, connections are ready or unready.

Your changes therefore should not make references to "handshakes" in the Selector code. And like I said in the other comment, the other metric does not seem useful, so I would think you wouldn't have to refer to "handshakes" anywhere in the patch.

p.s: The current selector code has references to handshakes, but only in comments (and that is wrong).

pnarayanan · 2016-05-23T18:52:17Z

ambry-network/src/main/java/com.github.ambry.network/Selector.java

@@ -72,10 +72,12 @@
  private final List<NetworkReceive> completedReceives;
  private final List<String> disconnected;
  private final List<String> connected;
+  private final List<String> connectionsPendingHandshake;


could this be made a set?

pnarayanan · 2016-05-23T19:08:46Z

ambry-network/src/main/java/com.github.ambry.network/Selector.java

@@ -72,10 +73,11 @@
  private final List<NetworkReceive> completedReceives;
  private final List<String> disconnected;
  private final List<String> connected;
+  private final Set<String> unreadycConnections;


pnarayanan · 2016-05-23T22:34:55Z

LGTM

vgkholla · 2016-05-24T07:25:09Z

ambry-network/src/main/java/com.github.ambry.network/Selector.java

  private final Time time;
  private final NetworkMetrics metrics;
  private final AtomicLong IdGenerator;
-  private AtomicLong activeConnections;
+  private AtomicLong numActiveConnections;


vgkholla · 2016-05-24T07:32:23Z

Two very minor comments. LGTM otherwise.

It looks like the tests cover the new code but could you add coverage details in the description and confirm that the new code is fully covered (including true and false for any conditionals that exist)?

pnarayanan · 2016-05-26T00:55:22Z

ambry-network/src/main/java/com.github.ambry.network/NetworkMetrics.java

+   * @param activeConnections count of current active connections
+   */
+  void initializeSelectorMetrics(final AtomicLong activeConnections) {
+    numActiveConnections = new Gauge<Long>() {


Haven't looked at the rest, but this Gauge is never registered as far as I can see.

Not related to your change though, but we should get this fixed.

Fixing Selector connected list for SSL connections

81a9256

nsivabalan mentioned this pull request May 16, 2016

Make selector connect() non-blocking for SSL connections #297

Closed

nsivabalan changed the title ~~Fixing the way Selector handles SSL connections wrt handshakes~~ Fixing the way Selector handles SSL connection handshakes May 16, 2016

xiahome reviewed May 16, 2016
View reviewed changes

Addressing comments

f22676e

nsivabalan force-pushed the SelectorSSLFix branch from a314af0 to f22676e Compare May 16, 2016 21:31

pnarayanan reviewed May 16, 2016
View reviewed changes

Minor fixes

789341e

nsivabalan force-pushed the SelectorSSLFix branch from ebcabcc to 789341e Compare May 17, 2016 00:40

xiahome reviewed May 17, 2016
View reviewed changes

vgkholla reviewed May 17, 2016
View reviewed changes

Fixing transmission close

a95ad0a

vgkholla reviewed May 20, 2016
View reviewed changes

renaming variable

ad1f146

nsivabalan force-pushed the SelectorSSLFix branch from 1c95e63 to ad1f146 Compare May 21, 2016 03:16

xiahome reviewed May 21, 2016
View reviewed changes

Addressing feedback

23b0454

nsivabalan force-pushed the SelectorSSLFix branch from a32cf78 to 23b0454 Compare May 21, 2016 16:10

pnarayanan reviewed May 21, 2016
View reviewed changes

pnarayanan reviewed May 23, 2016
View reviewed changes

Fixing docs

0b59f29

nsivabalan force-pushed the SelectorSSLFix branch from 271e4ea to 0b59f29 Compare May 23, 2016 18:59

pnarayanan reviewed May 23, 2016
View reviewed changes

Addressing Priyesh's comment

059e5ec

vgkholla reviewed May 24, 2016
View reviewed changes

Addressing gopal's comments

969f536

vgkholla merged commit 62fe306 into linkedin:master May 24, 2016

pnarayanan reviewed May 26, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing the way Selector handles SSL connection handshakes #310

Fixing the way Selector handles SSL connection handshakes #310

nsivabalan commented May 16, 2016 •

edited

Loading

xiahome May 16, 2016

nsivabalan May 16, 2016

pnarayanan May 16, 2016 •

edited

Loading

pnarayanan commented May 17, 2016

xiahome May 17, 2016

nsivabalan May 17, 2016

vgkholla May 17, 2016

vgkholla May 17, 2016

sriramsub May 17, 2016

nsivabalan May 17, 2016

xiahome commented May 17, 2016

xiahome commented May 17, 2016

vgkholla May 17, 2016

vgkholla May 20, 2016

pnarayanan commented May 20, 2016

vgkholla May 20, 2016

vgkholla commented May 20, 2016

nsivabalan commented May 21, 2016

pnarayanan commented May 21, 2016

xiahome May 21, 2016

nsivabalan May 21, 2016

xiahome commented May 21, 2016

nsivabalan commented May 21, 2016

nsivabalan commented May 21, 2016

pnarayanan May 21, 2016

nsivabalan May 23, 2016

pnarayanan commented May 21, 2016 •

edited

Loading

pnarayanan May 23, 2016

pnarayanan May 23, 2016

pnarayanan commented May 23, 2016

vgkholla May 24, 2016

vgkholla commented May 24, 2016

pnarayanan May 26, 2016

pnarayanan May 26, 2016

Fixing the way Selector handles SSL connection handshakes #310

Fixing the way Selector handles SSL connection handshakes #310

Conversation

nsivabalan commented May 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnarayanan May 16, 2016 • edited Loading

Choose a reason for hiding this comment

pnarayanan commented May 17, 2016

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

xiahome commented May 17, 2016

xiahome commented May 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnarayanan commented May 20, 2016

Choose a reason for hiding this comment

vgkholla commented May 20, 2016

nsivabalan commented May 21, 2016

pnarayanan commented May 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiahome commented May 21, 2016

nsivabalan commented May 21, 2016

nsivabalan commented May 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnarayanan commented May 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnarayanan commented May 23, 2016

Choose a reason for hiding this comment

vgkholla commented May 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsivabalan commented May 16, 2016 •

edited

Loading

pnarayanan May 16, 2016 •

edited

Loading

pnarayanan commented May 21, 2016 •

edited

Loading