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

What about concurrency? #2

Open
vanodevium opened this issue May 8, 2022 · 4 comments
Open

What about concurrency? #2

vanodevium opened this issue May 8, 2022 · 4 comments

Comments

@vanodevium
Copy link
Contributor

We have 3 way:

  1. we have to set up mutex
  2. we can write concurrency version of methods
  3. don't use goset in concurrency functionality (so sad)

What do you prefer?


Simple test case

func TestSet_Add_Concurrency(t *testing.T) {
	s := NewSet[int]()

	var wg sync.WaitGroup
	wg.Add(10)
	for i := 0; i < 10; i++ {
		go func(i int) {
			s.Add(i)
			wg.Done()
		}(i)
	}

	wg.Wait()

	require.Equal(t, 10, s.Len())
}

Result:

=== RUN   TestSet_Add_Concurrency
fatal error: concurrent map writes
@amit7itz
Copy link
Owner

I tend to say the prettiest way to add concurrency support is to add a concurrent-safe struct that wraps Set and adds a mutex lock before every operation (and defers an unlock).
This way people who need concurrency will have a convenient solution, without hurting the performance for those who don't need concurrency.

Methods in the new struct should be pretty simple, something like that:

func (c *ConcurrencySafeSet[T]) Add(items ...T) {
	c.lock.Lock()
	defer c.lock.Unlock()
	return c.set.Add(items...)
}

WDYT?

@vanodevium
Copy link
Contributor Author

@amit7itz Sounds good. I can try to do this. Wait new PR :)

@vanodevium
Copy link
Contributor Author

@amit7itz I did. Please look at

@amit7itz
Copy link
Owner

As discussed on the PR, right now there is no real demand for a concurrent safe Set, therefore we decided to postpone the work on this feature.
I keep the issue so in case anyone ever needs this, they can comment on their use case here and we'll consider adding a concurrent safe form of Set to the project.

Until then, Set can be used together with a standard lock to ensure concurrency safety.

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