Merge pull request #655 from ably/feature/issue-649

Improve channel map operations in respect of thread-safety
ably · Mar 3, 2021 · 27a5961 · 27a5961
2 parents fcb4a1a + e318ea3
commit 27a5961
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 26 deletions.
diff --git a/lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java b/lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
@@ -2,7 +2,6 @@
 
 import java.util.Iterator;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 
 import io.ably.lib.rest.AblyRest;
 import io.ably.lib.transport.ConnectionManager;
@@ -130,10 +129,6 @@ public interface Channels extends ReadOnlyMap<String, Channel> {
     }
 
     private class InternalChannels extends InternalMap<String, Channel> implements Channels, ConnectionManager.Channels {
-        private InternalChannels() {
-            super(new ConcurrentHashMap<String, Channel>());
-        }
-
         /**
          * Get the named channel; if it does not already exist,
          * create it with default options.
@@ -148,21 +143,24 @@ public Channel get(String channelName) {
         }
 
         @Override
-        public Channel get(String channelName, ChannelOptions channelOptions) throws AblyException {
-            Channel channel = map.get(channelName);
-            if (channel != null) {
+        public Channel get(final String channelName, final ChannelOptions channelOptions) throws AblyException {
+            // We're not using computeIfAbsent because that requires Java 1.8.
+            // Hence there's the slight inefficiency of creating newChannel when it may not be
+            // needed because there is an existingChannel.
+            final Channel newChannel = new Channel(AblyRealtime.this, channelName, channelOptions);
+            final Channel existingChannel = map.putIfAbsent(channelName, newChannel);
+
+            if (existingChannel != null) {
                 if (channelOptions != null) {
-                    if (channel.shouldReattachToSetOptions(channelOptions)) {
+                    if (existingChannel.shouldReattachToSetOptions(channelOptions)) {
                         throw AblyException.fromErrorInfo(new ErrorInfo("Channels.get() cannot be used to set channel options that would cause the channel to reattach. Please, use Channel.setOptions() instead.", 40000, 400));
                     }
-                    channel.setOptions(channelOptions);
+                    existingChannel.setOptions(channelOptions);
                 }
-                return channel;
+                return existingChannel;
             }
 
-            channel = new Channel(AblyRealtime.this, channelName, channelOptions);
-            map.put(channelName, channel);
-            return channel;
+            return newChannel;
         }
 
         @Override

diff --git a/lib/src/main/java/io/ably/lib/rest/AblyBase.java b/lib/src/main/java/io/ably/lib/rest/AblyBase.java
@@ -1,7 +1,5 @@
 package io.ably.lib.rest;
 
-import java.util.HashMap;
-
 import io.ably.annotation.Experimental;
 import io.ably.lib.http.AsyncHttpScheduler;
 import io.ably.lib.http.Http;
@@ -105,10 +103,6 @@ public interface Channels extends ReadOnlyMap<String, Channel> {
     }
 
     private class InternalChannels extends InternalMap<String, Channel> implements Channels {
-        InternalChannels() {
-            super(new HashMap<String, Channel>());
-        }
-
         @Override
         public Channel get(String channelName) {
             try {

diff --git a/lib/src/main/java/io/ably/lib/util/InternalMap.java b/lib/src/main/java/io/ably/lib/util/InternalMap.java
@@ -1,16 +1,23 @@
 package io.ably.lib.util;
 
-import java.util.Map;
 import java.util.Map.Entry;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
 
 import io.ably.lib.types.ReadOnlyMap;
 
+/**
+ * A map implemented using a {@link ConcurrentHashMap}. This class is a base class for other classes
+ * which are designed to be internal to the library, specifically as regards access to the map
+ * field.
+ *
+ * This class exposes a {@link ReadOnlyMap} which is safe to be exposed in our public API.
+ *
+ * @param <K> Key type.
+ * @param <V> Value type.
+ */
 public abstract class InternalMap<K, V> implements ReadOnlyMap<K, V> {
-    protected final Map<K, V> map;
-
-    public InternalMap(final Map<K, V> map) {
-        this.map = map;
-    }
+    protected final ConcurrentMap<K, V> map = new ConcurrentHashMap<>();
 
     @Override
     public final boolean containsKey(final Object key) {