taoensso / carmine

Redis client + message queue for Clojure
https://www.taoensso.com/carmine
Eclipse Public License 1.0
1.16k stars 131 forks source link

Unix Domain Socket (UDS) Support #126

Closed mavbozo closed 1 year ago

mavbozo commented 9 years ago

I modify your connections.clj code to use junixsocket https://code.google.com/p/junixsocket/ . And so far, I can connect carmine to redis with unix domain socket (uds).

For the connection spec, :host is the socket file path and :port is set to 0. ex:

{:pool {}
 :spec {:host "/tmp/redis.sock" :port 0}}

My code is not good yet. It's just a conditional check to :port. If port 0 then use junixsocket, else use regular socket.

You can see my commit to connections.clj in my fork here: https://github.com/mavbozo/carmine/commit/ea8ccd24fc003c5bddce040e4c49e137e5df4ccb

I'm looking suggestions to improve my code, documentation, etc so that my code meets the standard for pull request.

ptaoussanis commented 9 years ago

Hi there!

I'm a little swamped atm, and not familiar with junixsocket. Could you provide some info on the motivation behind this patch? What are the pros/cons of using junixsocket, etc.?

Thanks! :-)

mavbozo commented 9 years ago

I'm sorry if my request came at a inappropriate moment. Please postpone this issue if this issue takes away your precious time. I already made a somewhat working fork and I hope that maybe in the future I could use carmine directly with unix domain socket.

So, here are my motivations:

Redis accepts connection through Unix Domain Socket (UDS) as you can see in this configuration snippet https://github.com/antirez/redis/blob/902b877dc020df3f43b2f8179a85dc48144d0b20/redis.conf#L66-L71

# Specify the path for the Unix socket that will be used to listen for
# incoming connections. There is no default, so Redis will not listen
# on a unix socket when not specified.
#
# unixsocket /tmp/redis.sock
# unixsocketperm 700

There are some programmers--including me, who deploys redis-using clojure app to shared hosting services and UDS provide connection access security compared to port. Setting UDS where redis can listen to is the recommended approach in shared hosting scenario. Because, unlike port access that is open for another users in the same server, UDS permission can be restricted to a set of users.

As as side note, from redis benchmark http://redis.io/topics/benchmarks, antirez states that

Depending on the platform, unix domain sockets can achieve 
around 50% more throughput than the TCP/IP loopback (on Linux for instance). 

So, I was shocked knowing that carmine --which is currently the de facto clojure client for redis-- does not support UDS. Even Jedis still do not provide support for UDS. https://github.com/xetorthio/jedis/issues/492

For comparison, PHP, Python, Ruby have redis client library that support UDS.

Shouldn't a great redis client for clojure provides support for a feature that redis provides? :)

To implement UDS connection functionality, I use junixsocket which is a Java/JNI library that allows the use of Unix Domain Sockets from Java. When looking for java library that enables me to connect to uds, there are not many alternatives to choose from and junixsocket looks stable enough to use.

The cons of including this feature in carmine is:

ptaoussanis commented 9 years ago

I'm sorry if my request came at a inappropriate moment.

No, no - not at all. I'm just a little limited in the amount of time I can put in myself right now, so may require a little more assistance than usual if the goal is to get a PR in ASAP.

Thanks for all the clarification - that's exactly the info I was looking for. I've actually never used Unix domain sockets myself- know nothing about them. This sounds interesting, so am definitely on board with exploring a merge.

A few questions/requests:

  1. Could you provide some benchmarks comparing the performance of your fork to the same (but unforked) version of Carmine on your system? There's a taoensso.carmine.benchmarks utils ns.
  2. Is the commit at https://github.com/mavbozo/carmine/commit/ea8ccd24fc003c5bddce040e4c49e137e5df4ccb the only change necessary to get basic UDS support in?
  3. Should we expect Unix domain sockets to behave sensibly with the current connection pooling scheme?
  4. Is there support for connection timeouts?
  5. Anything else that you'd expect to want to modify to get fully-baked UDS support?

The only comments I have on your implementation so far:

  1. I may suggest something like a :uds? flag instead of a magic port number (0)? Or is port 0 a common idiom when using domain sockets elsewhere?
  2. I'm not keen on forcing a new dependency on all Carmine users, so we'd need to do this in a way which offers an optional/soft dependency. One possibility may be to setup a new connection protocol with a default (TCP) implementation, and an optional (UDS) implementation in a separate namespace that isn't compiled by default. This is something I'd be happy to handle myself if you can handle the rest.
mavbozo commented 9 years ago

I think UDS support will be bad for carmine. So, I want to drop my proposal.

I have tried your suggestions and the dangerous thing that I found is that there's no support for connections timeout.

I have tried to simulate busy redis-server using redis command DEBUG SLEEP 300 from redis-cli and the client try to ping using (car/ping). But the client just blocks and after 300 seconds the client returns PONG.

But if redis is down, the client throws exception.

Without timeout, there's no way the client drops connection if redis server is busy. If something wrong happened with redis-server which makes the redis-server busy for a very long time but still alive, the carmine client with uds connection happily blocks forever until the redis-server can accept connection or throw exception if the redis-server dies.

Without information in the form of timeout, we lose precious information on how to deal with busy redis-server. That's why I want to drop my proposal to include UDS support in carmine.

Thank you for considering my proposal.

ptaoussanis commented 9 years ago

Hi there, thanks for the follow-up!

I have tried your suggestions and the dangerous thing that I found is that there's no support for connections timeout.

How do the other clients (PHP, Python, Ruby) handle this?

mavbozo commented 9 years ago

Hi,

How do the other clients (PHP, Python, Ruby) handle this?

PHP, Python, Ruby clients throw connection timed-out error/exception in my test.

I think the java uds library that I use--junixsocket, does not implement connection timeout. I tried to look at the source code here and found that the timeout parameter does not passed to the C binding.

I try to look for alternatives but only junixsocket plays nice with maven (and leiningen).

ptaoussanis commented 9 years ago

Do you think it might be worth exploring creating our own (Clojure-side) timeout?

I.e. on connection(/pool) fetch start a timeout[1] that'll trigger after x msecs. Iff a server response is received in <x msecs, cancel the timeout.

[1] I'd be happy to see a first prototype with core.async for convenience. For dep-free production code, we could use something like http-kit's timer (implementation: https://github.com/http-kit/http-kit/blob/master/src/java/org/httpkit/timer/TimerService.java).

Again all this would only be worth exploring in detail if we're sure that the UDS performance is significantly better than TCP in practice.

mavbozo commented 9 years ago

The workaround is not worth exploring because--as you can see below in my tests, there are not much differences in performance between uds and tcp port.

uds benchmark

tcp port benchmark

I prefer waiting until junixsocket implements proper timeout.

ptaoussanis commented 9 years ago

Hmm, okay - so the difference seems to be about a 2x speed improvement for local unpipelined commands. The difference mostly goes away with pipelining (as to be expected).

Thanks for the benchmarks, those were very useful!

I prefer waiting until junixsocket implements proper timeout.

Sure, okay. I'll leave that decision up to you.

I'll leave this issue open in case you (or someone else) would like to investigate more later.

Thanks again for all your efforts + input. Cheers! :-)

ptaoussanis commented 1 year ago

Closing for now, someone's welcome to reopen again if there's renewed interest