It's not thread-safe, like at all.

[bmalehorn]

Madness! No goroutines allowed? This code panics:

package main

import (
    "github.com/mediocregopher/radix.v2/redis"
    "time"
)

const NTHREADS = 10

func main() {
    done := false
    client, err := redis.Dial("tcp", "localhost:6379")
    if err != nil {
        panic(err.Error())
    }
    for i := 0; i < NTHREADS; i++ {
        go func() {
            for !done {
                err := client.Cmd("INCR", "doom").Err
                if err != nil {
                    panic(err.Error())
                }
            }
        }()
    }
    time.Sleep(1 * time.Second)
    done = true
}

Does it make sense why that might fail? Multiple threads are calling writeTo(c.writeBuf) at the same time, weaving their input together.

To fix this, you gotta grab a mutex in Client.Cmd().

Also, the whole concept of the stateful PipeAppend/PipeResp doesn't make sense if two goroutines are trying to construct a pipe at the same time. Their commands might interleave as they're appending. Instead, you should be able to create a Pipe object that you can Pipe.Append() to, then send the requests and get all the responses with Pipe.Resps().

[mediocregopher]

The redis package is not thread safe, its a simple wrapper around a net.Conn and that's all. For multithreaded code I recommend looking at the pool package.

(Sorry for multiple comments, email formatting didn't do quite what I wanted)

So this is an issue which has shown up multiple times, the fault being my own because I guess the docs aren't clear enough on this. I'm thinking I'll add a big disclaimer in the little package description for redis in the README, as well as an even bigger one at the top of the redis package's godoc, so it'll appear on godoc.org. If I had done either of those, would it have been something you would have probably noticed? If not, do you have another suggestion of where to put it? Thanks! And sorry for the confusion

[bmalehorn]

Hm, I get it, my problems are solved with a pool with 1 goroutine.

Why do you expose the entire redis package? Why would anyone want a redis.Client over a redis.Pool of size 1?

It would make more sense to merge the redis and pool packages together. The normal way to make a client would create a pool of size 1:

type Client
  func Dial(network, addr string) (*Client, error)
  func DialFull(network, addr string, timeout time.Duration, size int) (*Client, error)
  func (c *Client) Close() error
  func (c *Client) Cmd(cmd string, args ...interface{}) *Resp
  func (c *Client) Pipe() *Pipe
  func (c *Client) ReadSubscribeResp() *Resp
type Pipe
  func (p *Pipe) Append(cmd string, args ...interface{})
  func (p *Pipe) Resp() *Resp

The average lazy idiot (aka me last night) is going to stop reading after the Cmd and Resp paragraph. "Great, I can send basic commands, I'm good to go!"

They're not going to want to see an angry message telling them this isn't the repo they're looking for. After all, this is the repo redis.io points to. It ought to work for the average lazy idiot, who probably calls the server concurrently.

[mediocregopher]

Hm, I get it, my problems are solved with a pool with 1 goroutine.

What exactly do you mean by 1 go-routine? The pool package itself does not spawn any go-routines, perhaps you meant a pool of size 1?

Why do you expose the entire redis package? Why would anyone want a redis.Client over a redis.Pool of size 1?

This would be rather pointless, I agree. Even in the example you originally posted a pool of size 5-10 would be much more appropriate.

In my view your proposed API is more complex than the current one, and would be more difficult for a newcomer to understand. Is there a reason (besides your admitted laziness) that having the pooling logic be separated into a separate entity is detrimental? The Pool type even has its own Cmd method on it, so for 90% of the time it's almost a drop in replacement.

The average lazy idiot (aka me last night) is going to stop reading after the Cmd and Resp paragraph. "Great, I can send basic commands, I'm good to go!"

If you'd like I've pushed up my documentation changes to the doc branch for now, if you'd like to take a look at them. The warning will occur just before the Cmd and Resp section, so I think (I'm hoping) it will be difficult to miss.

After all, this is the repo redis.io points to. It ought to work for the average lazy idiot, who probably calls the server concurrently.

Both of the repos that redis.io points to as its "recommended" (this and redigo) implement a simple, single-threaded connection with a separate pooling entity for multithreaded use. There are many reasons for organizing in this way, I can go into them if you like. I also disagree with the assertion that the average user wants to use a connection concurrently. I personally have written as many simply, single-threaded servers that just spawn up a connection and do work on it in a loop as I have ones which needed a pool.

[mediocregopher]

I've gone ahead and pushed up the changes to the documentation, let me know if you have any other issues