Repository: ably/ably-java
Issue: AblyBase.InternalChannels is not thread-safe

in the AblyBase class, the InternalChannels class holds a map of channels, which gets auto-populated on get.  however, the underlying data structure is a simple HashMap (which is not thread-safe).  when you do a get for a channel which does not currently exist in the map, a new one is created and then inserted into the underlying map.  this has three problems.

1. it isn't atomic, i.e. two different threads making this call for a new channel the first time could end up with two different instances of Channel.

1. the put() call on the underlying HashMap isn't thread safe, risking corrupting the underlying map.
1. modifying the options of the channel before returning it is not safe if the channel is being used by multiple threads concurrently.

note that 1,2 can be fairly simply and efficiently solved by using a ConcurrentHashMap and the computeIfAbsent method.

also note that AblyRealtime.InternalChannels has two of these problems as well (it does use a ConcurrentHashMap so 2 isn't an issue).



┆Issue is synchronized with this [Jira Bug](https://ably.atlassian.net/browse/SDK-650) by [Unito](https://www.unito.io)


Changed files:
lib/src/main/java/io/ably/lib/util/InternalMap.java, lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java, lib/src/main/java/io/ably/lib/rest/AblyBase.java

diff --git a/lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java b/lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
index a97205cb..0ac891ea 100644
--- 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 @@ package io.ably.lib.realtime;
 
 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 class AblyRealtime extends AblyRest implements AutoCloseable {
     }
 
     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 class AblyRealtime extends AblyRest implements AutoCloseable {
         }
 
         @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
index d5be54d0..72c4d039 100644
--- 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 abstract class AblyBase {
     }
 
     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
index 66d4c72c..d732e9b4 100644
--- 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) {