Automatically connecting to the local Redis instance can be misleading

[ignorabilis]

Hi, I have been using carmine for some time now and it's really nice. One thing I noticed though is that if the server connection is messed up, lets say instead of {:pool {} :spec {:uri (:redis "redis://192.168.0.1:6379")}} to be {} or "" carmine automatically tries to connect to the local Redis instance. This can be very misleading and sometimes this kind of issue can slip into production (if tested only locally). Furthermore the exception at that point can be misleading as well - ConnectException Connection refused java.net.PlainSocketImpl.socketConnect (PlainSocketImpl.java:-2) - which could force one to think that there is some issue with a firewall for example.

This may not be a bug, but I think it really needs an improvement.

Thanks for your time and keep up the good work.

[ptaoussanis]

Hi there! Sorry- just to clarify: what change/improvement are you suggesting exactly?

[ignorabilis]

Hi again, sorry for the late response.

I think that a java.net.ConnectException can be thrown. Or log a message to the REPL about not being able to connect and then connect to the local Redis instance. However for that one I am not sure how straightforward would be to implement.

In both cases the developer will know sooner than later that his code is wrong and is connecting to the wrong Redis instance.

[ptaoussanis]

No problem. To clarify: in what specific circumstance do you suggest throwing an exception or logging a message?

[ignorabilis]

Sorry for the "fast" responses.

Whenever the developer does something like:

(wcar "this is obviously wrong" (car/ping) (car/set "foo" "bar") (car/get "foo")) => ["PONG" "OK" "bar"]

The above example currently works and connects to the local Redis. What's more is that any value will be accepted there, even nil/false.

[ptaoussanis]

A nil opts is valid (=> use defaults). We could throw on other types (e.g. your string), but not sure how common an error that'd be?

But even if we throw on unexpected types, not sure how much that'd help since it wouldn't catch cases of a map type with opt typos, etc. (what I'm guessing may be more common?).

If this is a common cause of errors for you, I might suggest following the example in the README and defining a partial macro like:

(def server1-conn {:pool {<opts>} :spec {<opts>}}) ; See `wcar` docstring for opts
(defmacro wcar* [& body] `(car/wcar server1-conn ~@body))

Does all that make sense?

[ignorabilis]

We are using component and environ so basically we are retrieving the uri from the project file and then pass it around in different contexts. Whenever I fail to extract it from the context properly - for example when the function needs the redis connection but after a refactoring I am passing the whole context - things continue to work as expected until we test on another server.

The macro is nice for a small project but doesn't really fit in our environment.

[ptaoussanis]

We are using component and environ so basically we are retrieving the uri from the project file and then pass it around in different contexts.

That sounds good, that'd be the other way of avoiding typos here. I'm not sure I follow how you're ending up with an invalid conn opts map if you're bringing it in from something like component?

[ignorabilis]

It has happened to me after refactoring. Initially the function looked like this:

(defn uses-redis [{:keys [redis-conn something-else]}]
  ; redis-conn used
  ; something-else used
  )

and was used like this:

(uses-redis ctx)

After the refactoring it looked like this:

(defn uses-redis [redis-conn]
  ; redis-conn used
  )

But I forgot to change one usage and things continued to work locally. It's obviously my mistake, but it's a fairly stupid one that with the help of carmine could be caught immediately.

[ptaoussanis]

Hi there. Sorry, closing this as part of issue triage. Please feel to reopen if you have a specific PR that you'd like me to consider.

Cheers :-)