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

Add Unix domain socket support #1942

Closed
wants to merge 1 commit into from
+120 −15
Closed

Add Unix domain socket support #1942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,19 @@ cluster-enabled yes
cluster-config-file /tmp/redis_cluster_node5.conf
endef

# UDS REDIS NODES
define REDIS_UDS
daemonize yes
protected-mode no
port 0
pidfile /tmp/redis_uds.pid
logfile /tmp/redis_uds.log
unixsocket /tmp/redis_6379.sock
unixsocketperm 777
save ""
appendonly no
endef

#STUNNEL
define STUNNEL_CONF
cert = src/test/resources/private.pem
Expand All @@ -236,6 +249,7 @@ export REDIS_CLUSTER_NODE2_CONF
export REDIS_CLUSTER_NODE3_CONF
export REDIS_CLUSTER_NODE4_CONF
export REDIS_CLUSTER_NODE5_CONF
export REDIS_UDS
export STUNNEL_CONF
export STUNNEL_BIN

Expand Down Expand Up @@ -265,6 +279,7 @@ start: stunnel cleanup
echo "$$REDIS_CLUSTER_NODE3_CONF" | redis-server -
echo "$$REDIS_CLUSTER_NODE4_CONF" | redis-server -
echo "$$REDIS_CLUSTER_NODE5_CONF" | redis-server -
echo "$$REDIS_UDS" | redis-server -

cleanup:
- rm -vf /tmp/redis_cluster_node*.conf 2>/dev/null
Expand All @@ -291,6 +306,7 @@ stop:
kill `cat /tmp/redis_cluster_node3.pid` || true
kill `cat /tmp/redis_cluster_node4.pid` || true
kill `cat /tmp/redis_cluster_node5.pid` || true
kill `cat /tmp/redis_uds.pid` || true
kill `cat /tmp/stunnel.pid` || true
rm -f /tmp/sentinel1.conf
rm -f /tmp/sentinel2.conf
Expand Down
6 changes: 6 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
<redis-hosts>localhost:6379,localhost:6380,localhost:6381,localhost:6382,localhost:6383,localhost:6384,localhost:6385</redis-hosts>
<sentinel-hosts>localhost:26379,localhost:26380,localhost:26381</sentinel-hosts>
<cluster-hosts>localhost:7379,localhost:7380,localhost:7381,localhost:7382,localhost:7383,localhost:7384,localhost:7385</cluster-hosts>
<uds-hosts>/tmp/redis_6379.sock</uds-hosts>
<github.global.server>github</github.global.server>
</properties>

Expand All @@ -64,6 +65,11 @@
<type>jar</type>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>com.kohlschutter.junixsocket</groupId>
<artifactId>junixsocket-native-common</artifactId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like the idea of adding one more dependency* just to support one feature.

* compile time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I wasn't too mad about it as well, but thought it would be fine since it's a small dependency. So I think we have a few options (ordered by most to least favourite by me, but open to suggestions):

  • [My preferred suggestion] Make the dependency an optional one and do a reflection check to see if it is and if the feature can be supported
  • Shade and include the dependency (also since it's tiny), this way it would be self included and no need for consumers to worry
  • Accept the dependency as is now since it's tiny
  • Not include it at all and follow your below comments passing a socket straight in the constructor. It's my least preferred because I think the consumer shouldn't need to worry about creating/configuring the socket, specially that Jedis has some good defaults there. In the below comment you mention other sockets, any specific ones in mind?

(Sorry for the delayed response, this escaped me somehow...)

<version>2.0.4</version>
</dependency>

<dependency>
<groupId>junit</groupId>
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/redis/clients/jedis/BinaryClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import static redis.clients.jedis.Protocol.Keyword.STORE;
import static redis.clients.jedis.Protocol.Keyword.WITHSCORES;

import java.io.File;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -49,6 +50,10 @@ public BinaryClient(final String host) {
super(host);
}

public BinaryClient(final File unixDomainSocket) {
super(unixDomainSocket);
}

public BinaryClient(final String host, final int port) {
super(host, port);
}
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/redis/clients/jedis/BinaryJedis.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static redis.clients.jedis.Protocol.toByteArray;

import java.io.Closeable;
import java.io.File;
import java.io.Serializable;
import java.net.URI;
import java.util.AbstractMap;
Expand Down Expand Up @@ -56,6 +57,10 @@ public BinaryJedis(final String host) {
}
}

public BinaryJedis(final File unixDomainSocket) {
client = new Client(unixDomainSocket);
}

public BinaryJedis(final HostAndPort hp) {
this(hp.getHost(), hp.getPort());
}
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/redis/clients/jedis/Client.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static redis.clients.jedis.Protocol.toByteArray;

import java.io.File;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -30,6 +31,10 @@ public Client(final String host) {
super(host);
}

public Client(final File unixDomainSocket) {
super(unixDomainSocket);
}

public Client(final String host, final int port) {
super(host, port);
}
Expand Down
52 changes: 37 additions & 15 deletions src/main/java/redis/clients/jedis/Connection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package redis.clients.jedis;

import java.io.Closeable;
import java.io.File;
import java.io.IOException;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.net.SocketAddress;
import java.net.SocketException;
import java.util.ArrayList;
import java.util.List;
Expand All @@ -13,6 +15,8 @@
import javax.net.ssl.SSLSocket;
import javax.net.ssl.SSLSocketFactory;

import org.newsclub.net.unix.AFUNIXSocket;
import org.newsclub.net.unix.AFUNIXSocketAddress;
import redis.clients.jedis.commands.ProtocolCommand;
import redis.clients.jedis.exceptions.JedisConnectionException;
import redis.clients.jedis.exceptions.JedisDataException;
Expand All @@ -37,6 +41,7 @@ public class Connection implements Closeable {
private SSLSocketFactory sslSocketFactory;
private SSLParameters sslParameters;
private HostnameVerifier hostnameVerifier;
private File unixDomainSocket;

public Connection() {
}
Expand All @@ -45,6 +50,10 @@ public Connection(final String host) {
this.host = host;
}

public Connection(final File unixDomainSocket) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we take the Socket (and/or other objects) or a custom object which would create/connect a socket on demand? This would help to support many forms of sockets to be supported by Jedis at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See last point in above comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you mention other sockets, any specific ones in mind?

Not anything specific, rather generic.

I just meant, if we had such constructor, sockets like unix domain socket, ssl or even plain socket with different configuration from Jedis could be used by users.

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 just meant, if we had such constructor, sockets like unix domain socket, ssl or even plain socket with different configuration from Jedis could be used by users.

We can do that, but I think the question is should we expose that to the user as part of the library?

Jedis tries to do such constructs under the hood and expose other options (ssl, passwords, etc...) and sets some defaults for each of these, we would risk exposing this and making the library a bit harder (or a bit more confusing) to use.

It doesn't make much difference to my case by the way, so I am not opposing this, I just want to make sure you would be okay with that (minimal) risk.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we would risk exposing this and making the library a bit harder (or a bit more confusing) to use.

I'm not sure I follow you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a user, I would much rather set a minimal parameters (e.g. port number, UDS path, credentials), than creating/setting/maintaining a socket (more code and might not be setup with the best configuration unless I am an advanced user).

Not the strongest argument, but this is my intuition. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • About dependency:
    Most users are not enthusiastic about more dependencies. So I'm a bit reluctant on this.

  • About not easy option:
    At least it is an option which would allow avid users to avoid long routes (like extending Jedis/Connection classes, building custom Jedis). Let's take sendCommand method (coming in 3.1.0) as an example. It would require some coding for the user but would allow them to use new commands in Redis (which are not in Jedis) more easily without waiting for Jedis to include those.

These are my viewpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, sounds good, I will refactor this to allow creating it with a Socket object for advanced use-cases, will update this soon :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mina-asham Please take a look at #2036. WDYT?

this.unixDomainSocket = unixDomainSocket;
}

public Connection(final String host, final int port) {
this.host = host;
this.port = port;
Expand Down Expand Up @@ -166,19 +175,27 @@ public void setPort(final int port) {
public void connect() {
if (!isConnected()) {
try {
socket = new Socket();
// ->@wjw_add
socket.setReuseAddress(true);
socket.setKeepAlive(true); // Will monitor the TCP connection is
// valid
socket.setTcpNoDelay(true); // Socket buffer Whetherclosed, to
// ensure timely delivery of data
socket.setSoLinger(true, 0); // Control calls close () method,
// the underlying socket is closed
// immediately
// <-@wjw_add

socket.connect(new InetSocketAddress(host, port), connectionTimeout);
SocketAddress socketAddress;
if (unixDomainSocket == null) {
socket = new Socket();
// ->@wjw_add
socket.setReuseAddress(true);
socket.setKeepAlive(true); // Will monitor the TCP connection is
// valid
socket.setTcpNoDelay(true); // Socket buffer Whetherclosed, to
// ensure timely delivery of data
socket.setSoLinger(true, 0); // Control calls close () method,
// the underlying socket is closed
// immediately
// <-@wjw_add
socketAddress = new InetSocketAddress(host, port);
} else {
// unix domain socket doesn't support above options
socket = AFUNIXSocket.newStrictInstance();
socketAddress = new AFUNIXSocketAddress(unixDomainSocket);
}

socket.connect(socketAddress, connectionTimeout);
socket.setSoTimeout(soTimeout);

if (ssl) {
Expand All @@ -201,8 +218,13 @@ public void connect() {
inputStream = new RedisInputStream(socket.getInputStream());
} catch (IOException ex) {
broken = true;
throw new JedisConnectionException("Failed connecting to host "
+ host + ":" + port, ex);
if (unixDomainSocket == null) {
throw new JedisConnectionException("Failed connecting to host "
+ host + ":" + port, ex);
} else {
throw new JedisConnectionException("Failed connecting to socket "
+ unixDomainSocket, ex);
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/redis/clients/jedis/Jedis.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package redis.clients.jedis;

import java.io.File;
import java.net.URI;
import java.util.AbstractMap;
import java.util.ArrayList;
Expand Down Expand Up @@ -42,6 +43,10 @@ public Jedis(final String host) {
super(host);
}

public Jedis(final File unixDomainSocket) {
super(unixDomainSocket);
}

public Jedis(final HostAndPort hp) {
super(hp);
}
Expand Down
23 changes: 23 additions & 0 deletions src/test/java/redis/clients/jedis/tests/HostAndPortUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package redis.clients.jedis.tests;

import java.io.File;
import java.util.ArrayList;
import java.util.List;

Expand All @@ -10,6 +11,7 @@ public final class HostAndPortUtil {
private static List<HostAndPort> redisHostAndPortList = new ArrayList<HostAndPort>();
private static List<HostAndPort> sentinelHostAndPortList = new ArrayList<HostAndPort>();
private static List<HostAndPort> clusterHostAndPortList = new ArrayList<HostAndPort>();
private static List<File> redisUDSList = new ArrayList<File>();

private HostAndPortUtil(){
throw new InstantiationError( "Must not instantiate this class" );
Expand All @@ -36,13 +38,17 @@ private HostAndPortUtil(){
clusterHostAndPortList.add(new HostAndPort("localhost", 7383));
clusterHostAndPortList.add(new HostAndPort("localhost", 7384));

redisUDSList.add(new File("/tmp/redis_6379.sock"));

String envRedisHosts = System.getProperty("redis-hosts");
String envSentinelHosts = System.getProperty("sentinel-hosts");
String envClusterHosts = System.getProperty("cluster-hosts");
String envUDSHosts = System.getProperty("uds-hosts");

redisHostAndPortList = parseHosts(envRedisHosts, redisHostAndPortList);
sentinelHostAndPortList = parseHosts(envSentinelHosts, sentinelHostAndPortList);
clusterHostAndPortList = parseHosts(envClusterHosts, clusterHostAndPortList);
redisUDSList = parseUDSHosts(envUDSHosts, redisUDSList);
}

public static List<HostAndPort> parseHosts(String envHosts,
Expand Down Expand Up @@ -80,6 +86,19 @@ public static List<HostAndPort> parseHosts(String envHosts,
return existingHostsAndPorts;
}

public static List<File> parseUDSHosts(String envHosts, List<File> existingUDSHosts) {
if (null != envHosts && 0 < envHosts.length()) {

String[] hostDefs = envHosts.split(",");

List<File> envUDSHosts = new ArrayList<>();
for (String hostDef : hostDefs) {
envUDSHosts.add(new File(hostDef));
}
}
return existingUDSHosts;
}

public static List<HostAndPort> getRedisServers() {
return redisHostAndPortList;
}
Expand All @@ -91,4 +110,8 @@ public static List<HostAndPort> getSentinelServers() {
public static List<HostAndPort> getClusterServers() {
return clusterHostAndPortList;
}

public static List<File> getUDSServers() {
return redisUDSList;
}
}
18 changes: 18 additions & 0 deletions src/test/java/redis/clients/jedis/tests/UDSTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package redis.clients.jedis.tests;

import org.junit.Test;
import redis.clients.jedis.Jedis;

import java.io.File;

import static org.junit.Assert.assertEquals;

public class UDSTest {
protected static File udsHost = HostAndPortUtil.getUDSServers().get(0);
@Test
public void testCompareTo() {
Jedis jedis = new Jedis(udsHost);
assertEquals("PONG", jedis.ping());
jedis.close();
}
}