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

Add support for Unix sockets #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for Unix sockets #4

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
39 changes: 24 additions & 15 deletions src/connection.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module Redis
class Connection
include Commands

@socket : TCPSocket | OpenSSL::SSL::Socket::Client
@socket : TCPSocket | OpenSSL::SSL::Socket::Client | UNIXSocket

CRLF = "\r\n"

Expand All @@ -21,16 +21,31 @@ module Redis
# SSL connections require specifying the `rediss://` scheme.
# Password authentication uses the URI password.
# DB selection uses the URI path.
#
# To use Unix sockets: `socket:/path/to/redis.sock`
def initialize(@uri = URI.parse("redis:///"))
host = uri.host || "localhost"
port = uri.port || 6379
socket = TCPSocket.new(host, port)
socket.sync = false

# Check whether we should use SSL
if uri.scheme == "rediss"
socket = OpenSSL::SSL::Socket::Client.new(socket)
if uri.scheme == "socket"
Copy link
Owner

Choose a reason for hiding this comment

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

Thinking about this a bit more, I think the socket URL scheme may be a bit too generic a name — they're all sockets. 😄 Do you think it might make more sense to use unix as the URL scheme?

Copy link
Author

Choose a reason for hiding this comment

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

I see the point. Unfortunately, there is not a standard among the Redis clients to build on. Redis itself (e.g. in redis.conf) refers to it as "unixsocket". Maybe that is the clearest?

It is also more consistent with the other existing Redis client for Crystal: https://github.com/stefanwille/crystal-redis/blob/009e35fa40bbd518798afcc32c397402c6c6acb2/src/redis/connection.cr#L8

Copy link
Owner

Choose a reason for hiding this comment

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

Apologies for the wildly delayed reply here. I seem to have pulled this move:

oh no

If the Redis server recommends unixsocket, I imagine that's what most Redis clients also use, and consistency can be pretty important. Admittedly, since I almost always use TCP sockets for IPC even when Unix sockets are available, I don't have many personal opinions on how to use Unix sockets so my only real recommendation is least-surprise. 🙂

socket = UNIXSocket.new(uri.path)
socket.sync = false
db = "0"
Copy link
Owner

Choose a reason for hiding this comment

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

I see why you did it this way, but it seems like it would be impossible to use a DB slot other than 0 while using a Unix socket.

Copy link
Author

@philipp-classen philipp-classen Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Have to admit I only learned later what DB slots are. But agreed that it does not make sense to exclude the functionality depending on the connection type without any need.

I'll change it when I have some time.

Copy link
Owner

Choose a reason for hiding this comment

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

Following along the same lines as the other reply, if other Redis clients have opinions on how to select a different DB with a Unix socket URL, I'm happy to follow suit here.

else
host = uri.host || "localhost"
port = uri.port || 6379
socket = TCPSocket.new(host, port)
socket.sync = false

# Check whether we should use SSL
if uri.scheme == "rediss"
socket = OpenSSL::SSL::Socket::Client.new(socket)
socket.sync = false
end

# DB select
db = if {"", "/"}.includes?(uri.path)
"0"
else
uri.path[1..-1]
end
end

@socket = socket
Expand All @@ -42,12 +57,6 @@ module Redis
run({"auth", password})
end

# DB select
db = if {"", "/"}.includes?(uri.path)
"0"
else
uri.path[1..-1]
end
run({"select", db}) unless db == "0"
end
end
Expand Down