taoensso / carmine

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

wcar on lazy seq #138

Closed axel-angel closed 8 years ago

axel-angel commented 8 years ago

I've faced the problem where I had a lazy seq into wcar and it silently ignores commands:

  (car/wcar @words-redis-conn
            (car/incr (format "%s:%s" lang "ALL")) ; is executed
            (for [k ks]
              (car/incr (format "%s:%s" lang k))))) ; is not executed

I would propose to force lazy seq in the macro wcar or to throw an expection if this is by design. I don't see why it is silently ignored. For the time being I replaced with a mapv.

I read https://github.com/ptaoussanis/carmine/issues/95#issuecomment-50045596 but there is no justification why: is This a design choice or a bug? Why is that so?

Thanks and keep the good work!

ptaoussanis commented 8 years ago

Hi Axel,

Thanks and keep the good work!

You're welcome, thanks for saying so :-)

I would propose to force lazy seq in the macro wcar or to throw an expection if this is by design

I'm afraid neither of these would work very well or be particularly idiomatic for Clojure. How would you know what to realise? Just the top level? What about nesting? Is your intention to realise just Redis commands (i.e. things that'll have a comms side effect in the context of the Redis connection) - or everything? How could you tell which is which? What if you're serializing a data structure that contains lazy elements? What if your code depends on laziness for semantic reasons (e.g. for error logging, etc.).

I read #95 (comment) but there is no justification why: is This a design choice or a bug? Why is that so?

It's by design. If you're asking for a lazy sequence (as you're doing with for), you're introducing some intrinsic complexity that you need to be aware of. Clojure encourages (or at least used to) encourage laziness by default - which has (had) the unfortunate effect of introducing subtleties where people often didn't want/expect them (esp. beginners).

This isn't something that Carmine should (or even sensibly can) work around for you; you must be aware of laziness. Trying to automatically guess your intentions would be a dangerous road to go down.

Instead, I'd suggest getting into the habit of avoiding laziness unless you actually know you need/want it. This'd be a general recommendation, but doubly important when dealing with dynamic bindings where the interactions with laziness can become subtle.

Prefer doseq to for, mapv to map, etc. Clojure 1.7 also has a run! util which can be handy (and is very fast).

Does that help / make sense?

axel-angel commented 8 years ago

I see, yes it makes sense. What about throwing an exception then? In this case car/incr could and thus informs the user its command were ignored due to laziness. Is there a reason not to?

ptaoussanis commented 8 years ago

How would you know when to throw an exception? And what if it was your intention to have laziness?

axel-angel commented 8 years ago

Ah, I see that it's lazy thus it will never be released, so it cannot throws an exception on its on. But what about wcar that sees lazy seqs? Something that you pass to a function/macro explicitly shouldn't be ignored anyway.

ptaoussanis commented 8 years ago

But what about wcar that sees lazy seqs? Something that you pass to a function/macro explicitely shouldn't be ignored anyway.

I don't follow, could you rephrase?

shmish111 commented 8 years ago

This is interesting, I can't really see a situation where it would not be better to isolate what you give and get back from side-effecting code i.e. wcar. Clojure's prn forces evaluation of all arguments and to me that's useful as it encourages you to not just call prn on any data structure. I personally feel like it's better that you don't do much inside things like wcar however I see the same thing being done in with-open (that often causes newbie mistakes, I did it too). I am confused, clojure encourages many aspects of functional programming and yet seems to not mind about side-effects anywhere?

shmish111 commented 8 years ago

Of course, it might not be possible as the code is more complex than prn :-)

shmish111 commented 8 years ago

Oh and very nice library by the way, it's by far the best Redis lib I've used

axel-angel commented 8 years ago

What I meant is that you pass arguments to wcar but some are ignored due to laziness. This doesn't seem intuitive that wcar silently ignores them. It should throws an exception stating explicitely it expects only strict arguments, thus strict lists as well. Is there a problem by doing this?

ptaoussanis commented 8 years ago

Oh and very nice library by the way, it's by far the best Redis lib I've used

@shmish111 Thanks, appreciate that

Of course, it might not be possible as the code is more complex than prn :-)

Not so much more complex as more flexible. The decision to auto-realise all lazy seqs is certainly doable, just a bad idea IMO. It'd be slower and less flexible. In the case of pr, one could argue that there'd be no reasonable use cases for wanting real laziness since you're being explicit about the intention: print this thing.

With wcar, you're just establishing a Redis connection. What you do with that is up to you; it's not uncommon to have sophisticated logic w/in a wcar call. Removing that facility would be unfortunate.

The solution here is just to not ask for laziness if you don't want laziness.

shmish111 commented 8 years ago

I see, I should perhaps think of wcar as a context and that it doesn't actually do any IO (even if it does in reality). It is the car functions that should force/throw error if necessary (I imagine they do).

ptaoussanis commented 8 years ago

What I meant is that you pass arguments to wcar but some are ignored due to laziness.

No arguments are being ignored, they're just lazy. Laziness can be subtle, but has value. You're free to use it if you understand + want that subtlety - and you can avoid it if you don't (e.g. by using doseq instead of for in your example).

This doesn't seem intuitive that they wcar silently ignore them. It should throws an exception stating explicitely we except only strict arguments, thus strict lists as well. Is there a problem by doing this?

wcar accepts lazy arguments, and works with them the same way any other Clojure binding does. If you'd prefer to not have to deal with the complications introduced by laziness + bindings (which is a totally legitimate preference) - then you need to just not ask for laziness (i.e. don't use for).

ptaoussanis commented 8 years ago

I see, I should perhaps think of wcar as a context and that it doesn't actually do any IO (even if it does in reality).

Sure: it's a dynamic connection context in which Redis commands do IO. You can do mixed or non-IO work in that context though; that's quite idiomatic (+ convenient) for Carmine.

It is the car functions that should force/throw error if necessary (I imagine they do).

The functions will throw on obviously bad input (e.g. bad number of args) - but I'd encourage you to get away from the notion that APIs in general should do your lazy seq realisations for you. If you've asked for laziness, presumably you had a reason - and an API at Carmine's level would have no way of understanding your intentions so would need to guess.

Lazy content w/in a wcar body is valid: not an error/exception, and potentially valuable. But it carries the same complexity as any laziness+binding context.

My recommendation is to not ask for laziness if you don't need or want it (I seldom do). As a side benefit, your code will be semantically clearer and faster.

axel-angel commented 8 years ago

Thanks for your help. I now understand that wcar is not executing the commands, so it's not responsible for evaluating/forcing its argument/body. It just provides the context that commands require (like car/incr). The connection argument is passed behind-the-scene by this macro.

That was confusing and I see your point now. I don't have the skills in clojure yet to see how to solve the situation (1st post). Maybe a warning in the README about laziness, I don't know.

Thanks for the fast support!

ptaoussanis commented 8 years ago

Thanks for the fast support!

No problem, you're welcome!

That was confusing

It is pretty confusing :-) All for good reasons though (I'd argue) - and much of the complexity drops away if you don't ask for laziness.

Maybe a warning about laziness, I don't know.

Sure, would be happy to take a PR to add a warning about laziness to the wcar docstring, that sounds like a good idea.

Good luck, cheers! :-)