Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve channel map operations in respect of thread-safety #655

Merged
merged 3 commits into from Mar 3, 2021
Merged

Improve channel map operations in respect of thread-safety #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
723c43b
Always use a concurrent hash map for channel maps.
QuintinWillison Mar 2, 2021
ef3d136
Improve the channel get implementation so that it only makes a single…
QuintinWillison Mar 2, 2021
e318ea3
Remove unused imports.
QuintinWillison Mar 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 12 additions & 14 deletions lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
View file
Open in desktop
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use map.get(channelName) for concurrentHashmap too right?
https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ConcurrentHashMap.html#get(java.lang.Object)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your point. That is the method that this refactor is pretty much focussed on avoiding the use of. I've replaced two calls (one to get, followed by a subsequent conditional call to put, with a single call to atomic putIfAbsent as backed by the concurrent hash map).

Perhaps you need to re-read the issue to see what we're aiming to solve here. 🤔

Copy link
Collaborator

@sacOO7 sacOO7 Mar 3, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I was looking at the comment slight inefficiency of creating newChannel, if we use map.get(channelName), we can avoid it. I feel it's a tradeoff, in most cases, the client will try to get an existing channel rather than creating a new one.


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
Expand Down
6 changes: 0 additions & 6 deletions lib/src/main/java/io/ably/lib/rest/AblyBase.java
View file
Open in desktop
@@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
19 changes: 13 additions & 6 deletions lib/src/main/java/io/ably/lib/util/InternalMap.java
View file
Open in desktop
@@ -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) {
Expand Down