Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
rework the code a little to make it more clear, and clean up a couple tests. #4890
Conversation
ericsnowcurrently
reviewed
Mar 24, 2016
| - started chan struct{} | ||
| +type StringForwarder struct { | ||
| + messages chan<- string | ||
| + done chan<- struct{} | ||
| discardCount uint64 |
ericsnowcurrently
Mar 24, 2016
Contributor
Minor: Why export the type now? Why not the fields too? TBH, neither really seem important enough to be part of this patch.
ericsnowcurrently
reviewed
Mar 24, 2016
| +// loop pipes messages from the messages channel into the callback function. It | ||
| +// closes started to signal that it has begun, and will clean itself up when | ||
| +// done is closed. | ||
| +func loop(started chan struct{}, done <-chan struct{}, messages <-chan string, callback func(string)) { |
ericsnowcurrently
Mar 24, 2016
Contributor
Why make this a func instead of a method? Is there some re-use here that I'm not seeing? Having to pass in done and messages when they are also fields of the type is confusing. Passing in callback is good.
natefinch
Mar 24, 2016
Contributor
The reason I made loop into a stanadalone function is that it's a goroutine... it doesn't have any state it is storing, and it shouldn't be implied that it is. There's two separate concerns, a goroutine looping over channel, and a type that can send messages to that channel. Linking them to the same object is conceptually incorrect, and unnecessary. Plus it means the goroutine has receive-only halves of done and messages, making it clear that it should not (and cannot) be the one closing them.
That being said I'll put it back to a method, since that seems to be the more usual way to do it here.
|
Loop not being a method isn't how our other worker-like things work, and I'd like Stop() to be reentrant, but otherwise LGTM. |
ericsnowcurrently
reviewed
Mar 24, 2016
| + } | ||
| + for i := 0; i < 4; i++ { | ||
| + wg.Add(1) | ||
| + go f(wg) |
ericsnowcurrently
reviewed
Mar 24, 2016
| close(stop) | ||
| + count := forwarder.Stop() |
ericsnowcurrently
Mar 24, 2016
Contributor
Swapping these two lines is the actual fix, right? While all the clean-up is good, it may have been better to do it separately. If the patch would have been just moving the one line down then this would have been a trivial ship-it.
|
Other than minor points, LGTM. Did I mention how I hate doing reviews on Github? :P |
natefinch
reviewed
Mar 24, 2016
| } | ||
| } | ||
| - c.Errorf("managed to send too many messages without being stopped") |
natefinch
Mar 24, 2016
Contributor
This is the actual fix.... removing the limited length for-loop. It's not really testing anything important.
|
fixes-1560203 |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$nobutreally$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
natefinch commentedMar 24, 2016
Fix for bug https://bugs.launchpad.net/juju-core/+bug/1560203
So, honestly, the fix for this bug was "don't do that". The test was not failing because it found a race condition, it was failing because one goroutine was going a lot faster than another goroutine. This is probably just a difference in the scheduler on gccgo and/or the load on the machine. There doesn't seem to be any need to test that stop actually stops the goroutines in some amount of time... that's not what the test is for, so I just removed that error and made the loops in the goroutine run forever. To make it a little more stressful, I created 4 goroutines sending messages, and added code to wait for the goroutines to start up. In theory the time.sleep(1ms) should do that, but it's not guaranteed, so I used a waitgroup to ensure that they're actually going.
I cleaned up the production code some... it was an unexported type being returned from an exported function, which is a no-no. I also removed some extraneous fields from StringForwarder that aren't actually needed by the type itself. I renamed Receive to Forward, since it's a forwarder, not a receiver, and that value was being assigned to a field named forward anyway.
Finally, I removed the re-entrant code on Stop, since it would be easy to think that Stop was now able to be called from multiple goroutines, which it is not (unless we added mutexes to it). Being able to call Stop multiple times is not something we need to do, and so there's no reason to allow it. if we later need a way to get the count after stopping, it's easy enough to add a Count() method, but for now, we don't need it.