Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A code for SAFE use across concurrent goroutines? #78

Closed
frederikhors opened this issue Jun 11, 2022 · 3 comments
Closed

A code for SAFE use across concurrent goroutines? #78

frederikhors opened this issue Jun 11, 2022 · 3 comments

Comments

@frederikhors
Copy link

frederikhors commented Jun 11, 2022

I'm trying to use for the first time the ulid package, but using the below code I'm getting panics everywhere.

Can you help me understand why and how to write code SAFE for concurrent use across goroutines?

REPL: https://go.dev/play/p/Ysr8cCgF44n

package main

import (
    // "math/rand"
    "crypto/rand"
    "fmt"
    "log"
    "sync"
    "time"

    "github.com/oklog/ulid/v2"
)

func main() {
    var wg sync.WaitGroup

    // entropy := ulid.Monotonic(rand.New(rand.NewSource(time.Now().UnixNano())), 0)
    entropy := ulid.Monotonic(rand.Reader, 0)

    workers := 5

    ids := make([]ulid.ULID, 0, 10)

    for w := 0; w < workers; w++ {
        wg.Add(1)

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

            for i := 0; i < cap(ids)/workers; i++ {
                ids = append(ids, ulid.MustNew(ulid.Timestamp(time.Now()), entropy))
            }
        }(w)
    }

    wg.Wait()

    seen := make(map[ulid.ULID]bool)

    for _, id := range ids {
        if seen[id] {
            fmt.Println(id)
            log.Fatal("duplicate")
        }
        seen[id] = true
    }
}
@tsenart
Copy link
Contributor

tsenart commented Jun 11, 2022

Your code has two data races:

  1. Appending to ids mutates ids, so you need to make sure that is synchronized.
  2. ulid.MustNew mutates entropy, so that also needs to be synchornized

Here's a version that achieves both of those with a single mutex. In this specific example this makes concurrency unnecessary since it essentially makes all the code in the worker sequential, but in a real world example where the worker code does more than this synchronized section, it would be potentially different.

https://go.dev/play/p/3tEYbCaRvMf

@frederikhors
Copy link
Author

Thanks. I was fixing that code because it was a mistake for the ids := append part.

Using go run -race . with your code I get data race too and I can fix is using this instead:

mu.Lock()
for i := 0; i < cap(ids)/workers; i++ {
  ids = append(ids, ulid.MustNew(ulid.Timestamp(time.Now()), entropy))
}
mu.Unlock()

But my question was about using ulid with common libraries such ent, gqlgen and using functions like:

func NewID() string {
	return ulid.MustNew(ulid.Timestamp(time.Now()), entropy).String()
}

Is there a safe way to use the package?

What I'm looking for is an example for the code to be used for creating IDs used by common libraries that use various goroutines internally.

With my example I tried to replicate that functioning.

Made myself clear?

@frederikhors
Copy link
Author

frederikhors commented Jun 11, 2022

Superseded by #79.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants