Transaction API is confusing

[benjamin-hodgson]

I recently answered a Stack Overflow question in which the questioneer was confused about the meaning and usage of the Queued type. I agree with them: the Queued type and the multi-parameter RedisCtx class do seem unintuitive.

In my answer, I argued that a monadic interface is not the right abstraction for Redis transaction batches. I also gave an outline of a simpler Applicative interface on top of RedisTx, which does away with the confusing elements of the current design:

newtype RedisBatch a = RedisBatch (R.RedisTx (R.Queued a))
-- being a transactional batch of commands to send to redis

instance Functor RedisBatch where
    fmap = liftA

instance Applicative RedisBatch where
    pure x = RedisBatch (pure (pure x))
    (RedisBatch rf) <*> (RedisBatch rx) = RedisBatch $ (<*>) <$> rf <*> rx

Is there an appetite for changing the hedis API to remove the Queued type and expose a purely-applicative interface for RedisTx? It'd be a breaking API change, and the work would likely involve changing the MonadRedis/RedisCtx hierarchy.

I'd be keen to make a pull request for this, but I wanted to gauge interest before I get started so as not to waste anyone's time.

[qrilka]

how do you think Applicative could be enough here?
What about scenario like:

runRedis $ multiExec $ do
  val <- get "foo"
  set "bar" val

?

[informatikr]

First, I vehemently disagree that Queued is "confusing". It is what Redis commands return while in multi/exec mode. You even gave an example on Stackoverflow that illustrated the point of Queued.

That said, you are of course right that RedisTx should technically be an Applicative. But:

• The combination of RedisTx being a Monad and the Queued return value "is lika an Applicative", because you can only get the Queuedvalue by exec'ing the transaction.
• do-notation is nicer than applicative operators. It makes it possible to keep commands and their replies "linked" in the code, instead of referring to them positionally. The situation will change, when GHC 8 with ApplicativeDo is in widespread use.

What to do about it:

• Most important, no hasty changes, please. The current design has worked well for years, and nobody complained about it to me, until today.
• Making RedisTx an Applicative "requires" ApplicativeDo, in my opinion. So it will be feasible in the not-too-far future.
• There are rough plans to change the Redis return type from Either Reply a to just a (and aborting the runRedis call on Redis and network errors. Perhaps this and the Applicative-RedisTx could be done together/uniformly.
• Naming is important. I would not throw out "Queued" and introduce "Batch". Queued is a Redis term, batch isn't.

[informatikr]

@qrilka, your scenario is impossible in Redis (and Hedis).

[benjamin-hodgson]

@qrilka Thank you for perfectly illustrating what's confusing about RedisTx! That code looks correct, but it won't compile. val will be a Queued ByteString (not a ByteString) so you can't pass it to set.

[benjamin-hodgson]

@informatikr I agree that ApplicativeDo would make RedisTx computations nicer to write.

The name RedisBatch was just the name I used for the SO example; I had to come up with something other than RedisTx that meant roughly the same thing.

To be clear: my proposal is to keep the name RedisTx but remove the Queued type and the Monad instance from the public API. I do stand by my point that this is simpler and easier to learn than the current API. There are fewer moving parts.

I'd like to help out on this: if you have thoughts on the order in which things have to be done then that'd be great!

[informatikr]

@benjamin-hodgson Don't be snide. Removing a Monad instance will not remove the need that people read the documentation of their database, or a compiler error will be the least of their worries. And if it were sufficient that the code "looks correct" we would all be using JavaScript and wonder why get("bar") returns "Queued".

To also be clear, I think your proposal is good and we should keep it in mind (and this issue open).

[benjamin-hodgson]

I'm sorry for coming across as snide. It wasn't my intention. What do you think are the next steps?

[qrilka]

@benjamin-hodgson I don't think that you were snide here - it looks that I was carried away by thoughts on Applicative vs Monad completely forgetting the context itself :)

[informatikr]

No worries.

Unless @k-bx has a different view on this, I would postpone any changes to the multiExec API until

1. ApplicativeDo is widely available (at least in stack LTS, not sure if any later makes sense)
2. We have other major breaking changes. Specifically, switching from Either Reply a to a + Exception might influence RedisTx anyway. So it would be good to at least think about both of these together.

So if you want to work on this right now, a concept for point 2. above would be a step forward. This would be either changing RedisCtx or replacing it by something else, such as a type family (the aim being that commands still change their type and behavior according to the context Redis vs. RedisTx).

I realize, this might not be very clear and helpful. But this is because, right now, there is no clear plan how to move forward. I hope it helps, anyway.

[k-bx]

I fully agree with @informatikr 's point in last comment. Removing Either seems more important (and possibly breaking change, for which we might want to look for other alternatives like keeping old interface and providing a new one).

I'd wait for ApplicativeDo as well for this Applicative migration.

[benjamin-hodgson]

I spent a few hours hacking away at replacing the Either Reply a return type with an exception monad. I learned something interesting: it breaks auto-pipelining.

Here's an example of a sequence of commands that will be pipelined with the current version of hedis:

m = do
    x <- get "foo"
    y <- get "bar"
    return $ somePureComputation x y

Compare with a computation that cannot be pipelined:

m = do
    x <- get "foo"
    case x of
         l@(Left _) -> return $ l
         Right Nothing -> return (Left "no foo")
         Right (Just result) -> set "bar" result

Scrutinising x (to learn whether the command succeeded) forces us to wait until the get command has returned before sending the set request. Representing Redis as an ExceptT causes this pattern-matching to happen on every call to >>=, meaning that replies are always awaited before continuing.

Note that pipelined commands behave very differently from non-pipelined commands with regards to errors. In the computation...

m = do
    x <- lpop "foo"
    set "bar" "baz"

... what should happen if lpop "foo" fails (perhaps because the value at "foo" was not a list)? Presently, hedis will continue to execute the set command (possibly destructively); you have to pattern-match x explicitly if you don't want to continue on error. (If Redis lived inside an exception monad, the computation would fail fast and the set command would not be executed.) This is not obvious, and I'd even argue that it's a latent bug. It took me several hours of head-scratching to realise this was happening, and I don't see any notes about this error-handling behaviour in the hedis documentation.

So pipelining is another example of a feature that can only correctly support an Applicative interface. What do you think is the right way to go ahead?

1. One could introduce another type RedisPipeline (or similar) supporting an Applicative interface, and to remove command pipelining from Redis.
   • This has all the usual downsides of explicit pipelining: it increases the API surface, and will affect the out-of-the-box performance of Redis.
2. An alternative would be to implement Redis's Applicative interface with pipelining and the Monad interface without.
   • I don't like this idea because it becomes difficult to predict which behaviour you're signing up to, especially if you're using ApplicativeDo. This can be dangerous - see my comment re error handling above.
3. Another option would be to throw an IO exception when Redis reports an error.
   • This means you can't recover from application-level errors (such as LPOPping a non-list) from inside the Redis monad.

My preference would be the first option, because it helps users fall into the "pit of success". Fail-fast error handling is almost always what you want, and I think you should have to explicitly opt out (by working in a different monad) if you want the extra performance of pipelining.

What do you think? Can you think of any other options? Happy to move this discussion to a new issue if you prefer.

[k-bx]

@benjamin-hodgson I think option 3 is the way to go, actually. While I would by default stick to something like option 1, I suspect this would affect performance too much on one hand, and I think redis's commands are simple enough to state that having error-responses is something which should happen very rare (and be fixed quickly).

So if that is implementeable without breaking pipeline – I'd go with throwing IO exceptions.

[informatikr]

Regarding the three options:

1. Automatic pipelining is the feature of Hedis. It's the coolest thing about it. In fact, coming up with the idea is the single reason why I started Hedis: No other client (in any language) did it at the time, and I thought that lazy IO should make it possible. Turns out it does. As far as I am concerned, automatic pipelining is not going away.
2. I recently tried using the Applicative/Monad/ApplicativeDo combo for automatic pipelining. It's works nicely, except I didn't find a way to also limit the number of requests "in the pipeline". To avoid this causing a space leak, I basically had to re-create large parts of the lazy-IO version and nothing was gained. Of course there might be a better implementation than my proof-of-concept. Another argument against this is that it requires the user to activate ApplicativeDo, which means automatic pipelining doesn't "Just Work".
3. As @k-bx said, this is the option we will probably use: Throw exceptions and catch them in runRedis. An option to make error messages nicer might be to include the Redis command that caused the error in the exception, to help debugging.

[benjamin-hodgson]

Automatic pipelining is fast and cool, but not safe, due to the way it affects error handling. I understand that you're unwilling to change the default (though I'd love to change your mind!), but I think this pitfall should at least be documented. I'd also like it to be convenient to turn off (that is, it should not require a ton of case expressions) if a user prefers the slow-but-safe behaviour.

In the mean time, I'll look into whether raising exceptions in IO affects pipelining.