Fix bugs in aggregator and filters, add initial tests

[juliusv]

From the individual commits:

Close() was not synced through the main dispatcher loop, so it could close
channels that were currently being written to by methods called from said
dispatcher loop. This leads to a crash. Instead, Close() now writes a
closeRequest, which is handled in the dispatcher.

Fix regex filters to match complete string.
If someone specifies

service = "foo-service"

...they probably don't want it to match:

service = "foo-servicebar"

[matttproud]

If you return, it won't drain pending requests.

[juliusv]

Hm, how important is this? If we want to stay with the old structure, I could add the foo, closed := <- ... to the remaining cases and check the value of closed before doing anything to fix the immediate crash. However, we also have timers that eventually put requests on these channels. So I guess Close() would need to stop these timers first and then close the channels they write into. I hope I'm not overlooking more, as this is getting a bit confusing.

[juliusv]

There's another problem: when a channel is closed, the select case for it will not only be triggered once, but repeatedly in the for loop. So the closed counter is incremented multiple times for the same channel, until this has happened often enough that the for loop will exit (even though other channels might still be open). So we must either track closedness separately per channel or we ignore proper draining for now and fix it up later.

[matttproud]

Ignore proper draining for now with a giant BUG. I am surprised that it
re-emits closed when the damn thing is closed.
Am 19.07.2013 15:04 schrieb "juliusv" notifications@github.com:

In manager/aggregator.go:

@@ -288,8 +288,11 @@ func (a *Aggregator) Dispatch(s SummaryReceiver) {
log.Println("Deleting expired aggregation instance", a)
a.Aggregates[fp].Close()
delete(a.Aggregates, fp)
+

•   case req := <-a.closeRequests:
  

•       a.closeInternal()
  

•       req.done <- true
  

•       return
  

There's another problem: when a channel is closed, the select case for it
will not only be triggered once, but repeatedly in the for loop. So the
closed counter is incremented multiple times for the same channel, until
this has happened often enough that the for loop will exit. So we must
either track closedness separately per channel or we ignore proper draining
for now and fix it up later.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2/files#r5291343
.

[juliusv]

Added a BUG comment.

[matttproud]

How was the Aggregator writing to itself by sending requests back to itself? This sounds problematic.

By virtue of not closing the channels and waiting for them to actual return closed in the second value when getting from them, we have no draining. :-(

[matttproud]

1. Make a testAggregatorScenario struct{} with these inputs.
2. Make a function for it (s *testAggregatorScenario) test(i int, t *testing.T) {} that runs the actual tests.
3. Have the TestAggregator body just test via looping through each scenario.
   1. Maybe have a scenario setup and scenario close method the ensure cleanup.

[juliusv]

Did 1, 2, and 3, didn't do 3.1 because it would just add unneeded code (storing aggregator in struct, then closing from close()).

[juliusv]

That's a good question why it crashes, since actually all the external calls are synced. When I revert the closing commit, the tests fail with:

panic: runtime error: invalid memory address or nil pointer dereference

Looking at it.

[juliusv]

@matttproud Ah, I think that merely by closing the channels, the select{} got another event for each of them and the logic didn't check whether the channel was closed before executing whatever action was tied to that channel.

[juliusv]

@matttproud Is this ok to merge for now? I have more stuff in the pipeline :)

[matttproud]

Please do. 👍