vardius / gollback

Go asynchronous simple function utilities, for managing execution of closures and callbacks
MIT License
123 stars 13 forks source link

Race method failed in some case. #2

Closed YoghurtFree closed 2 years ago

YoghurtFree commented 2 years ago
import (
    "context"
    "errors"
    "fmt"
    "time"

    "github.com/vardius/gollback"
)

func main() {
    r, err := gollback.Race(
        context.Background(),
        func(ctx context.Context) (interface{}, error) {
            time.Sleep(3 * time.Second)
            return 1, nil
        },
        func(ctx context.Context) (interface{}, error) {
            time.Sleep(2 * time.Second)
            return nil, errors.New("failed")
        },
        func(ctx context.Context) (interface{}, error) {
            return nil, errors.New("failed")
        },
    )
    fmt.Println(r,err)
}

excepted out: 1,nil display out: nil, failed

In short, many goroutines race, if the last goroutine run faster than others and return an error, finally, an error will return in race function, this result is not correct.

YoghurtFree commented 2 years ago

@vardius @MikeSouza

vardius commented 2 years ago

in your example the first one (without error) will take about 3 seconds, where the other ones will finish earlier with an error

are you expecting here the first one to finish without error win the race ? this may lead to some important errors being surpassed and never handled

YoghurtFree commented 2 years ago

in your example the first one (without error) will take about 3 seconds, where the other ones will finish earlier with an error

are you expecting here the first one to finish without error win the race ? this may lead to some important errors being surpassed and never handled

thx to your reply.

func main() {
    r, err := gollback.Race(
        context.Background(),
                // funcA
        func(ctx context.Context) (interface{}, error) {
            time.Sleep(2 * time.Second)
            return 1, nil
        },
                // funcB
        func(ctx context.Context) (interface{}, error) {
            time.Sleep(1 * time.Second)
            return nil, errors.New("failed")
        },
                // funcC
        func(ctx context.Context) (interface{}, error) {
            time.Sleep(3 * time.Second)
            return nil, errors.New("failed")
        },
    )
    fmt.Println(r,err)
}

excepted out: 1,nil display out: 1,nil

funcA runs about 2s and return without error funcB runs about 1s and return an error funcC runs about 3s and return an error According to my thought, as long as there is one func return without error, the race will return without an error by the comment of race fucntion: image

besides, you say "this may lead to some important errors being surpassed and never handled", only the last goroutine error can be considered important error, other error will be surpassed.

vardius commented 2 years ago

Yea you are right! Sorry I forgot how it was meant to work, anyway I have pushed a fix with some extra test cases. See https://github.com/vardius/gollback/commit/ba02a068c2c18f48164ccffdbc86295f034b9de8

If you feel like improving some methods feel free to issue a pull request!

YoghurtFree commented 2 years ago

thx to your good job to fix race function when I add a test case for race function, the unit case failed and output

fatal error: all goroutines are asleep - deadlock!

package gollback

import (
    "context"
    "errors"
    "reflect"
    "testing"
    "time"
)

func TestRace(t *testing.T) {
    tests := []struct {
        name    string
        fns     []AsyncFunc
        want    interface{}
        wantErr bool
    }{
        {name: "no callback", want: nil, wantErr: true, fns: []AsyncFunc{}},
        {name: "first non error wins", want: 3, wantErr: false, fns: []AsyncFunc{
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(3 * time.Second)
                return 1, nil
            },
            func(ctx context.Context) (interface{}, error) {
                return nil, errors.New("failed")
            },
            func(ctx context.Context) (interface{}, error) {
                return 3, nil
            },
        }},
        {name: "all functions error out but first one to finish", want: 1, wantErr: false, fns: []AsyncFunc{
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(1 * time.Second)
                return 1, nil
            },
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(2 * time.Second)
                return nil, errors.New("failed")
            },
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(3 * time.Second)
                return nil, errors.New("failed")
            },
        }},
        {name: "all functions error out but middle one to finish", want: 1, wantErr: false, fns: []AsyncFunc{
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(2 * time.Second)
                return 1, nil
            },
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(1 * time.Second)
                return nil, errors.New("failed")
            },
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(3 * time.Second)
                return nil, errors.New("failed")
            },
        }},
        {name: "all functions errors but last one to finish", want: 1, wantErr: false, fns: []AsyncFunc{
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(3 * time.Second)
                return 1, nil
            },
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(2 * time.Second)
                return nil, errors.New("failed")
            },
            func(ctx context.Context) (interface{}, error) {
                return nil, errors.New("failed")
            },
        }},
        {name: "all functions return an error", want: nil, wantErr: true, fns: []AsyncFunc{
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(3 * time.Second)
                return nil, errors.New("failed")
            },
            func(ctx context.Context) (interface{}, error) {
                time.Sleep(2 * time.Second)
                return nil, errors.New("failed")
            },
            func(ctx context.Context) (interface{}, error) {
                return nil, errors.New("failed")
            },
        }},
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got, err := Race(context.Background(), tt.fns...)
            if (err != nil) != tt.wantErr {
                t.Errorf("Race() error = %v, wantErr %v", err, tt.wantErr)
                return
            }
            if !reflect.DeepEqual(got, tt.want) {
                t.Errorf("Race() got = %v, want %v", got, tt.want)
            }
        })
    }
}

In summary, when all fns return an error ,the race function will deadlock besides, I pull an request to solve this bug.

vardius commented 2 years ago

Merged: https://github.com/vardius/gollback/pull/3 thanks for figuring this out