Why does RedisClient use 'currentSend' to form an adhoc mutex?

[ericchapman]

Looking at the current RedisClient code, "currentSend" is being used to create an adhoc mutex that prevents multiple Redis requests from being lined up on the same eventLoop. This violates the foundation of an eventLoop architecture and will GREATLY complicate anyone's code that is trying to process allot of items in the same event loop using data in Redis.

That being said, I created a test case that runs back to back commands that would normally trigger the assertion. When I remove the 'currentSend' code and run my test case, it passes with flying colors, as does all of the tests. Code here

https://github.com/ericchapman/redis/commit/9e92f5cb1b09630ceccc3e77744b959585162693

So my question is, why is this code even here? If there is some corner case that it is trying to protect against, we should create a test case that would fail without this code so we can actually "fix the problem" vs. adding a band aid that violates the eventLoop process flow.

[tanner0101]

Check out the conversation about this here: https://github.com/vapor/redis/pull/133. This PR (https://github.com/vapor/redis/pull/135) attempted to make the change suggested here and it did not work.

The next version of this package will be based off of https://github.com/vapor/redis-kit which does not have these issues and supports proper pipelining.

[ericchapman]

Thanks @tanner0101. I figured there was something that was preventing that from being removed, just couldn't figure out what.

What is the timeline on the next release that fixes that? Days? Weeks? Months? I have a project I am working on that will need this, just trying to understand how long this will be so I can make a design decision.

[tanner0101]

We're still early days with the release, so it will be a couple of months at least.

[ericchapman]

Thanks @tanner0101. I'll await the updated library

[ericchapman]

@tanner0101 Hey Tanner. I am resurfacing and back in Vapor :). I spent last night going through all of the Redis/Vapor repos to see where you guys are at. It looks like you have everything ready for Vapor 4.0.

Is this a good time to try and intercept this or do you guys need more time? I can also help write helpers or whatever if need be.

[Mordil]

@ericchapman If you want to help, I was planning on backporting the implementation of RedisCommandHandler from RediStack to Vapor's implementation as a minor semver bump.

It's essentially the same as NIO's RequestResponseHandler but with extra bells & whistles, so NIO's is probably a better source to copy/paste into this repo, with just making changes for it to work with NIO 1.0

[ericchapman]

@Mordil Nathan, your timing is impeccable. :) I have been trying to see if I could get RediStack running with Vapor. I haven't been able to even get the package manager to complete without freezing (tried all sorts of different versions, on swift 5.1, etc.). I would love to be a guinea pig and get this going in Vapor.

Also, I think I have a way I can help you and meet in the middle. In preparation for switching redis libraries, I abstracted my redis code into such a way where it is a protocol. See small snippet below

import Vapor

public protocol RedisApiData {

    /// --------------------------- Methods to Define -------------------------------

    var redisToArray: [RedisApiData]? { get }
    var redisToString: String? { get }
    var redisToInt: Int? { get }
    var redisToDouble: Double? { get }

    /// -----------------------------------------------------------------------------

    var redisToBool: Bool { get }
    var redisToStringArray: [String] { get }
    var redisToStringStringTuple: (String, String)? { get }
    var redisToStringStringTupleArray: [(String, String)] { get }
    var redisToStringStringDictionary: [String:String] { get }
    var redisToDoubleStringTuple: (Double, String)? { get }
    var redisToDoubleStringTupleArray: [(Double, String)] { get }
}

public protocol RedisApi {

    /// --------------------------- Methods to Define -------------------------------

    /// Variable to return the worker
    ///
    var worker: Container { get }

    /// Method to send commands
    ///
    /// - note: The method should also handle returning the errors
    ///
    /// - parameters:
    ///   - command - The Redis commands
    ///   - args - The arguments associated with the command
    ///
    /// - returns: Redis response
    ///
    func send(command: String, args: [String]) -> Future<RedisApiData>

    /// Method to send pipeline of commands
    ///
    /// - note: The method should also handle returning the errors
    ///
    /// - parameters:
    ///   - command - The Redis commands
    ///   - args - The arguments associated with the command
    ///
    /// - returns: Redis responser fo each command
    ///
    func send(pipeline commands: [[String]]) -> Future<[RedisApiData]>

    /// ------------------------------- Redis Methods -------------------------------

    /// BLPOP key [key ...] timeout
    /// BLPOP is a blocking list pop primitive
    ///
    /// [SPEC](https://redis.io/commands/blpop)
    ///
    func blpop(keys: [String], timeout: Int) -> Future<(String, String)?>
    ...

I then implement all of the helper methods (well, subset but will do all at some point) in the protocol extension EXCEPT the ones above in the "Methods to define" sections. I then created a wrapper object that implements "RedisApi" by defining the 2 send methods. See an example a RedisClient below

import Vapor
import Redis

/// This is a wrapper class to abstract the Redis library from the rest
/// of the code
public class RedisWrapper {

    /// The redis connection for this client
    let redis: RedisClient

    /// The worker for this client
    public let worker: Container

    /// Constructor
    ///
    init(on: Container, redis: RedisClient) {
        self.worker = on
        self.redis = redis
    }

    /// Creates a redis pooled connection
    ///
    /// - note: The class is designed to create a pooled connection and
    ///   auto-release it automatically on deinit
    ///
    static func create(on worker: Container) -> Future<RedisWrapper> {
        return worker.requestPooledConnection(to: .redis).map(to: RedisWrapper.self) { redis in
            return RedisWrapper.init(on: worker, redis: redis)
        }
    }

    /// On deinit, release the pooled connection
    ///
    deinit {
        do {
            try self.worker.releasePooledConnection(self.redis, to: .redis)
        } catch {
            print(error)
        }
    }
}

///------------------------------------- Wrap Redis Client --------------------------------

/// Implement 'RedisApiData' protocol
///
extension RedisData: RedisApiData {
    public var redisToArray: [RedisApiData]? { return self.array }
    public var redisToString: String? { return self.string }
    public var redisToInt: Int? { return self.int }
    public var redisToDouble: Double? {
        if let string = self.string {
            return Double(string)
        }
        return nil
    }
}

/// Implement 'RedisApi' protocol
///
extension BackgrounderRedis: RedisApi {
    public func send(command: String, args: [String]) -> Future<RedisApiData> {
        let data = ([command] + args).map { RedisData(bulk: $0) }
        return self.redis.send(RedisData.array(data)).map(to: RedisApiData.self, {
            (data: RedisData) -> RedisApiData in
            return data as RedisApiData
        })
    }

    public func send(pipeline commands: [[String]]) -> Future<[RedisApiData]> {
        let commandArray = commands.map { RedisData.array( $0.map { RedisData(bulk: $0) }) }
        return self.redis.send(pipeline: commandArray).map(to: [RedisApiData].self, {
            (data: [RedisData]) -> [RedisApiData] in
            return data as [RedisApiData]
        })
    }
}

This also allowed me to wrap "pipeline" and "multi" in the protocol itself, they will just call "send(pipeline:)". So as you can see, it turns this entire "helper method" inclusion into an integration exercise vs. a "from scratch" every time.

Anyways, long story short, I was thinking about releasing this protocol as an implementation agnostic set of Redis helper methods available via SPM and then just documenting how to bind it to different libraries. Then from your perspective, you can either integrate the protocol into the library via SPM OR just point people to it and they can do it in their own projects.

What do you think?

[Mordil]

@ericchapman It sounds like an interesting idea as a separate package for letting users abstract over a few different implementations if they need some stability in the interim.

FYI RediStack is not compatible with Vapor 3. It requires NIO 2 & Swift 5, so Vapor 4 will be the earliest it can be incorporated - which RedisKit & Redis (this repo) will handle doing.

[ericchapman]

@Mordil Thanks. I re-read your post and I originally I thought you were going to port RediStack back, hence why I was suggesting I could add helpers, but sounds like you are just going to copy NIORedis into this repo and get it working. That would also be fantastic. Ultimately I just need a Redis version in Vapor 3 that can handle multiple transactions in flight on the same worker/event loop.

I'll also still code my abstraction as planned. We can review once I am done and you can decide if it's helpful to you or not. No worries.

Just out of curiosity, what sort of timeline are you thinking? Weeks? Months? If the timeline is pretty far out, I can take a stab at it on a branch with some guidance.

[Mordil]

@ericchapman I went ahead and did this last night: https://github.com/vapor/redis/pull/149

If you want to rely on that branch, and test it out for your use case - please do and report back any errors you encounter!

[ericchapman]

@mordil I commented on the PR but will here as well. My tests all passed switching to this branch. I'll try some more complex cases, but so far this is looking good to me. Thank you very much for porting that into this library

[Mordil]

This should be resolved as of #149 and Redis 3.4.0