Fix race condition accessing ServerDescription

Further reduce the possibility of introducing a race
by removing the Server#getDescription method

JAVA-3911

Author: jyemin

driver-core/src/main/com/mongodb/internal/connection/DefaultServer.java

@@ -124,12 +124,6 @@ public void onResult(final InternalConnection result, final Throwable t) {
         });
     }
 
-    @Override
-    public ServerDescription getDescription() {
-        isTrue("open", !isClosed());
-        return description;
-    }
-
     @Override
     public void resetToConnecting() {
         serverStateListener.stateChanged(new ChangeEvent<>(description, ServerDescription.builder()

driver-core/src/main/com/mongodb/internal/connection/Server.java

@@ -18,7 +18,6 @@
 
 import com.mongodb.annotations.ThreadSafe;
 import com.mongodb.internal.async.SingleResultCallback;
-import com.mongodb.connection.ServerDescription;
 
 /**
  * A logical connection to a MongoDB server.
@@ -27,13 +26,6 @@
  */
 @ThreadSafe
 public interface Server {
-    /**
-     * Gets the description of this server.  Implementations of this method should not block if the server has not yet been successfully
-     * contacted, but rather return immediately a @code{ServerDescription} in a @code{ServerConnectionState.CONNECTING} state.
-     *
-     * @return the description of this server
-     */
-    ServerDescription getDescription();
 
     /**
      * <p>Gets a connection to this server.  The connection should be released after the caller is done with it.</p>

driver-core/src/main/com/mongodb/internal/connection/SingleServerCluster.java

@@ -30,6 +30,7 @@
 import com.mongodb.event.ServerDescriptionChangedEvent;
 
 import static com.mongodb.assertions.Assertions.isTrue;
+import static com.mongodb.connection.ServerConnectionState.CONNECTING;
 import static java.lang.String.format;
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
@@ -55,7 +56,8 @@ public SingleServerCluster(final ClusterId clusterId, final ClusterSettings sett
         // In other words, we are leaking a reference to "this" from the constructor.
         synchronized (this) {
             this.server = createServer(settings.getHosts().get(0), new DefaultServerDescriptionChangedListener());
-            publishDescription(server.getDescription());
+            publishDescription(ServerDescription.builder().state(CONNECTING).address(settings.getHosts().get(0))
+                    .build());
         }
     }
 

driver-core/src/test/functional/com/mongodb/internal/binding/AsyncSingleConnectionBinding.java

@@ -48,6 +48,8 @@ public class AsyncSingleConnectionBinding extends AbstractReferenceCounted imple
     private AsyncConnection writeConnection;
     private volatile Server readServer;
     private volatile Server writeServer;
+    private volatile ServerDescription readServerDescription;
+    private volatile ServerDescription writeServerDescription;
 
     /**
      * Create a new binding with the given cluster.
@@ -79,6 +81,7 @@ public AsyncSingleConnectionBinding(final Cluster cluster, final ReadPreference
             public void onResult(final ServerTuple result, final Throwable t) {
                 if (t == null) {
                     writeServer = result.getServer();
+                    writeServerDescription = result.getServerDescription();
                     latch.countDown();
                 }
             }
@@ -88,6 +91,7 @@ public void onResult(final ServerTuple result, final Throwable t) {
             public void onResult(final ServerTuple result, final Throwable t) {
                 if (t == null) {
                     readServer = result.getServer();
+                    readServerDescription = result.getServerDescription();
                     latch.countDown();
                 }
             }
@@ -162,14 +166,14 @@ public void getReadConnectionSource(final SingleResultCallback<AsyncConnectionSo
         if (readPreference == primary()) {
             getWriteConnectionSource(callback);
         } else {
-            callback.onResult(new SingleAsyncConnectionSource(readServer, readConnection), null);
+            callback.onResult(new SingleAsyncConnectionSource(readServerDescription, readConnection), null);
         }
     }
 
     @Override
     public void getWriteConnectionSource(final SingleResultCallback<AsyncConnectionSource> callback) {
         isTrue("open", getCount() > 0);
-        callback.onResult(new SingleAsyncConnectionSource(writeServer, writeConnection), null);
+        callback.onResult(new SingleAsyncConnectionSource(writeServerDescription, writeConnection), null);
     }
 
     @Override
@@ -182,18 +186,19 @@ public void release() {
     }
 
     private final class SingleAsyncConnectionSource extends AbstractReferenceCounted implements AsyncConnectionSource {
-        private final Server server;
+        private final ServerDescription serverDescription;
         private final AsyncConnection connection;
 
-        private SingleAsyncConnectionSource(final Server server, final AsyncConnection connection) {
-            this.server = server;
+        private SingleAsyncConnectionSource(final ServerDescription serverDescription,
+                                            final AsyncConnection connection) {
+            this.serverDescription = serverDescription;
             this.connection = connection;
             AsyncSingleConnectionBinding.this.retain();
         }
 
         @Override
         public ServerDescription getServerDescription() {
-            return server.getDescription();
+            return serverDescription;
         }
 
         @Override

driver-core/src/test/functional/com/mongodb/internal/binding/SingleConnectionBinding.java

@@ -21,7 +21,7 @@
 import com.mongodb.internal.connection.Cluster;
 import com.mongodb.internal.connection.Connection;
 import com.mongodb.internal.connection.NoOpSessionContext;
-import com.mongodb.internal.connection.Server;
+import com.mongodb.internal.connection.ServerTuple;
 import com.mongodb.internal.selector.ReadPreferenceServerSelector;
 import com.mongodb.internal.selector.WritableServerSelector;
 import com.mongodb.internal.session.SessionContext;
@@ -39,9 +39,8 @@ public class SingleConnectionBinding implements ReadWriteBinding {
     private final ReadPreference readPreference;
     private final Connection readConnection;
     private final Connection writeConnection;
-    private final Server readServer;
-    private final Server writeServer;
-
+    private final ServerDescription readServerDescription;
+    private final ServerDescription writeServerDescription;
     private int count = 1;
 
     /**
@@ -62,10 +61,12 @@ public SingleConnectionBinding(final Cluster cluster) {
     public SingleConnectionBinding(final Cluster cluster, final ReadPreference readPreference) {
         notNull("cluster", cluster);
         this.readPreference = notNull("readPreference", readPreference);
-        writeServer = cluster.selectServer(new WritableServerSelector()).getServer();
-        writeConnection = writeServer.getConnection();
-        readServer = cluster.selectServer(new ReadPreferenceServerSelector(readPreference)).getServer();
-        readConnection = readServer.getConnection();
+        ServerTuple writeServerTuple = cluster.selectServer(new WritableServerSelector());
+        writeServerDescription = writeServerTuple.getServerDescription();
+        writeConnection = writeServerTuple.getServer().getConnection();
+        ServerTuple readServerTuple = cluster.selectServer(new ReadPreferenceServerSelector(readPreference));
+        readServerDescription = readServerTuple.getServerDescription();
+        readConnection = readServerTuple.getServer().getConnection();
     }
 
     @Override
@@ -100,7 +101,7 @@ public ConnectionSource getReadConnectionSource() {
         if (readPreference == primary()) {
             return getWriteConnectionSource();
         } else {
-            return new SingleConnectionSource(readServer, readConnection);
+            return new SingleConnectionSource(readServerDescription, readConnection);
         }
     }
 
@@ -112,23 +113,23 @@ public SessionContext getSessionContext() {
     @Override
     public ConnectionSource getWriteConnectionSource() {
         isTrue("open", getCount() > 0);
-        return new SingleConnectionSource(writeServer, writeConnection);
+        return new SingleConnectionSource(writeServerDescription, writeConnection);
     }
 
     private final class SingleConnectionSource implements ConnectionSource {
+        private final ServerDescription serverDescription;
         private final Connection connection;
-        private final Server server;
         private int count = 1;
 
-        SingleConnectionSource(final Server server, final Connection connection) {
-            this.server = server;
+        SingleConnectionSource(final ServerDescription serverDescription, final Connection connection) {
+            this.serverDescription = serverDescription;
             this.connection = connection;
             SingleConnectionBinding.this.retain();
         }
 
         @Override
         public ServerDescription getServerDescription() {
-            return server.getDescription();
+            return serverDescription;
         }
 
         @Override

driver-core/src/test/functional/com/mongodb/internal/connection/SingleServerClusterTest.java

@@ -103,15 +103,15 @@ public void shouldGetServerWithOkDescription() {
         setUpCluster(getPrimary());
 
         // when
-        Server server = cluster.selectServer(new ServerSelector() {
+        ServerTuple serverTuple = cluster.selectServer(new ServerSelector() {
             @Override
             public List<ServerDescription> select(final ClusterDescription clusterDescription) {
                 return getPrimaries(clusterDescription);
             }
-        }).getServer();
+        });
 
         // then
-        assertTrue(server.getDescription().isOk());
+        assertTrue(serverTuple.getServerDescription().isOk());
     }
 
     @Test

driver-core/src/test/unit/com/mongodb/internal/connection/BaseClusterSpecification.groovy

@@ -116,7 +116,7 @@ class BaseClusterSpecification extends Specification {
 
         expect:
         cluster.selectServer(new ReadPreferenceServerSelector(ReadPreference.secondary()))
-                .server.description.address == firstServer
+                .serverDescription.address == firstServer
     }
 
     def 'should use server selector passed to selectServer if server selector in cluster settings is null'() {
@@ -132,7 +132,7 @@ class BaseClusterSpecification extends Specification {
         factory.sendNotification(thirdServer, REPLICA_SET_PRIMARY, allServers)
 
         expect:
-        cluster.selectServer(new ServerAddressSelector(firstServer)).server.description.address == firstServer
+        cluster.selectServer(new ServerAddressSelector(firstServer)).serverDescription.address == firstServer
     }
 
     def 'should timeout with useful message'() {
@@ -190,7 +190,7 @@ class BaseClusterSpecification extends Specification {
 
         expect:
         cluster.selectServer(new ReadPreferenceServerSelector(ReadPreference.primary()))
-                .server.description.address == thirdServer
+                .serverDescription.address == thirdServer
 
         cleanup:
         cluster?.close()
@@ -272,10 +272,10 @@ class BaseClusterSpecification extends Specification {
         factory.sendNotification(firstServer, REPLICA_SET_SECONDARY, allServers)
 
         when:
-        def server = selectServerAsyncAndGet(cluster, firstServer)
+        def serverDescription = selectServerAsync(cluster, firstServer).getDescription()
 
         then:
-        server.description.address == firstServer
+        serverDescription.address == firstServer
 
         cleanup:
         cluster?.close()
@@ -300,8 +300,8 @@ class BaseClusterSpecification extends Specification {
         factory.sendNotification(thirdServer, REPLICA_SET_SECONDARY, allServers)
 
         then:
-        secondServerLatch.get().description.address == secondServer
-        thirdServerLatch.get().description.address == thirdServer
+        secondServerLatch.getDescription().address == secondServer
+        thirdServerLatch.getDescription().address == thirdServer
 
         cleanup:
         cluster?.close()
@@ -361,6 +361,7 @@ class BaseClusterSpecification extends Specification {
         def serverLatch = new ServerLatch()
         cluster.selectServerAsync(new ServerAddressSelector(serverAddress)) { ServerTuple result, MongoException e ->
             serverLatch.server = result != null ? result.getServer() : null
+            serverLatch.serverDescription = result != null ? result.serverDescription : null
             serverLatch.throwable = e
             serverLatch.latch.countDown()
         }
@@ -370,6 +371,7 @@ class BaseClusterSpecification extends Specification {
     class ServerLatch {
         CountDownLatch latch = new CountDownLatch(1)
         Server server
+        ServerDescription serverDescription
         Throwable throwable
 
         def get() {
@@ -379,5 +381,13 @@ class BaseClusterSpecification extends Specification {
             }
             server
         }
+
+        def getDescription() {
+            latch.await()
+            if (throwable != null) {
+                throw throwable
+            }
+            serverDescription
+        }
     }
 }

driver-core/src/test/unit/com/mongodb/internal/connection/TestServer.java

@@ -90,7 +90,6 @@ public int getConnectCount() {
         return connectCount;
     }
 
-    @Override
     public ServerDescription getDescription() {
         return description;
     }