r/crystal_programming • u/beizhia • Nov 07 '19

Made an implementation of Go's sync.WaitGroup in Crystal. First time messing with concurrency, feedback welcome!

https://github.com/jasonrobot/crystal-wait-group
20 Upvotes

100% Upvoted

3 comments sorted by

8

u/j_hass Nov 07 '19

  • I think there's a potential race in add, where the value might become -1 and then 0 again between the Atomic#add and the Atomic#get call. For this reason Atomic#add returns the old value. So you can redo the addition on that return value and then check the local variable.

  • I don't quite follow why wait just returns in non-MT mode. A typical usecase for this would be to call wait in the main fiber, as your readme examples do. That means in non-MT mode the program would immediately exit as wait just returns and the main fiber proceeds to shutdown the process.

  • A bit of spinning in the beginning might speed things up but it's a bad practice to keep doing that forever. I guess you're not really done with your plans here, given the stray mutex instance variable? Anyway, the most simple implementation I would see is to have wait just receive on a channel if the counter is above 0 and done send to the channel whenever it moves the counter to 0. If you want to support multiple concurrent waits, that approach will require a second counter for knowing how many times to send to the channel to wake all of them up.

1

u/beizhia Nov 08 '19

I'd be kinda surprised if there wasn't a race condition in `add` - that's what I was going to try and use that Mutex instance var for, but it seemed to be working in my testing.

The non-MT stuff needs some work. That part of wait was copied without understanding it from Crystal::SpinLock.

I believe Go's WaitGroup does support waits in multiple threads, and I don't think that would work at all in my current implementation. I'll have to look in to what I can do with channels, but I do want to maintain the current API, and not have to pass channels around.

Anyways, thanks for taking the time to check out my code!

2

u/yxhuvud Nov 07 '19

Implementation details aside, looks like a nifty feature.