bsm / redislock

Simplified distributed locking implementation using Redis
Other
1.45k stars 157 forks source link

Does not redislock work properly with multiple Go gin API at a time? #40

Closed iamatsundere closed 2 years ago

iamatsundere commented 2 years ago

Hi, I made an API that receive file

func main() {
    route := gin.Default()
    route.POST("/upload",func(c *gin.Context) {
            //process file
        clientL := redis.NewClient(&redis.Options{
                        Network:  "tcp",
                        Addr:     "localhost:6379",
                        DB:       0,
                    })
            defer clientL.Close()
            locker := redislock.New(clientL)
            ctx := context.Background()
            lock, err := locker.Obtain(ctx, "file", 5000*time.Millisecond, nil)
            if err != nil {
                fmt.Println(err)
                return
            }
            defer lock.Release(ctx)
                fmt.Println("pass")
                //return response
    })
    route.Run("0.0.0.0:8080")
}

Normally, I think when there are multiple requests at the same time want to upload a file. They must pause at the line I obtain the lock, if my lock is released or timeout, they will continue. It it doesn't, the request will fail. But when I make a client that sends 4 requests at the same time to my API, they can obtain the lock for all of them and it continues for next calls.

Did I do something wrong or I misunderstand something in this concept, please help!

musclechen commented 2 years ago

This redislock has no retry strategy by default. Your code looks like it will simply return false if the lock was not acquired. I don't quite understand what you said 'they can obtain the lock for all of them and it continues for next calls.' In theory, four threads grab the lock at the same time, and only one thread will succeed in grabbing the lock. I don't know if you are using postman. The four requests sent by postman at the same time are actually executed serially, that is, B will be executed after A is executed. You can add one row like this after the 'Obtain'. fmt.Print(lock) time.sleep(10*time.Second)' If you can see four threads at the same time print the lock acquired successfully. May be a bug of redislock.

iamatsundere commented 2 years ago

Yeah, sorry for the part I've talked about 'pausing'. What I mean is the lock would failed or process if it is obtainable or not. But whenever I call the API, my lock can be obtained. If I want to wait the lock to be obtained, I will use a loop to wait.

About 4 requests simultaneously, I am using a client written in ReactJS that makes multiple request at a time.

dim commented 2 years ago

I don't know about gin but I just created a tiny HTTP server and it seems to be doing what it says on the tin :)

package main

import (
    "io"
    "net/http"
    "time"

    "github.com/bsm/redislock"
    "github.com/go-redis/redis/v8"
)

func main() {
    client := redis.NewClient(&redis.Options{
        Addr: ":6379",
        DB:   9,
    })
    defer client.Close()

    locker := redislock.New(client)
    http.HandleFunc("/", func(rw http.ResponseWriter, r *http.Request) {
        ctx := r.Context()
        lock, err := locker.Obtain(ctx, "file", 5*time.Second, nil)
        if err == redislock.ErrNotObtained {
            http.Error(rw, "Busy", http.StatusTooManyRequests)
            return
        } else if err != nil {
            http.Error(rw, err.Error(), http.StatusInternalServerError)
            return
        }
        defer lock.Release(ctx)

        select {
        case <-ctx.Done():
            http.Error(rw, "Gone", http.StatusGone)
            return
        case <-time.After(200 * time.Millisecond):
            // assume request takes 200ms to complete
        }

        io.WriteString(rw, "OK")
    })

    http.ListenAndServe(":8080", nil)
}

When I run the client code below, I get:

request #02: 429 "Busy"
request #03: 429 "Busy"
request #01: 200 "OK"
request #05: 429 "Busy"
request #06: 429 "Busy"
request #04: 200 "OK"
request #08: 429 "Busy"
request #09: 429 "Busy"
request #07: 200 "OK"
request #10: 200 "OK"
package main

import (
    "bytes"
    "fmt"
    "io"
    "log"
    "net/http"
    "sync"
    "time"
)

func main() {
    wg := sync.WaitGroup{}
    defer wg.Wait()

    for i := 0; i < 10; i++ {
        wg.Add(1)

        go func(n int) {
            defer wg.Done()

            time.Sleep(time.Duration(n) * 100 * time.Millisecond)

            resp, err := http.Get("http://localhost:8080")
            if err != nil {
                log.Fatalln(err)
            }

            b, _ := io.ReadAll(resp.Body)
            fmt.Printf("request #%02d: %d %q\n", n, resp.StatusCode, bytes.TrimSpace(b))
        }(i + 1)
    }
}
iamatsundere commented 2 years ago

Ok tks, the problem is in my code :)