heal.Client tests and minor fixes

[lobkovilya]

Added unit-tests for heal.Client, fix minor issues

[lobkovilya]

@edwarnicke please take a look, I moved test to heal_test packet and covered case when connection monitor breaks

[edwarnicke]

Have you rebased recently? We discovered a couple of days ago the linter wasn't running properly. In fixing it we corrected a ton of linting. There are a lot of changes in this PR that look like your ID is applying linting to an older-than-that-fix instance of the code, which makes it seem like more change than there is.

[edwarnicke]

Its probably worth talking a bit about the lifecycle of heal.NewClient. heal.NewClient basically is part of a Client instance, meaning its pointing to a single grpc.ClientConnInterface. It may have many connections Requested, simultaneously open, and then at various times Closed in its lifecycle. We only really want to make sure we aren't leaking things when heal.NewClient is really finished with its life (meaning we are for some reason discarding the struct). The old finalizer approach was trying to do that. It seems that the new approach here is moving to a per-connection lifecycle. I don't quite follow why, but would be interested to understand it :)

[lobkovilya]

@edwarnicke There were a bug, in lazyInit and lazyTearDown I made wrong checks. Now it looks like:

func (f *healClient) lazyInit(id string) {
	f.updateExecutor.SyncExec(func() {
		if len(f.connectionIDs) == 0 {
			f.updateExecutor.AsyncExec(f.init)
		}
		f.connectionIDs[id] = true
	})
}

func (f *healClient) lazyTearDown(id string) {
	f.updateExecutor.SyncExec(func() {
		delete(f.connectionIDs, id)
		if len(f.connectionIDs) == 0 {
			f.updateExecutor.AsyncExec(f.tearDown)
		}
	})
}

So they start monitoring after the first Request and stop it after the last Close. If after the last Close we get new Request then monitoring will be started again.

@haiodo has also nice idea - create context that will exist per-chain. So we will have our usual context that is per-requers and new one per-chain. That allows us to cancel any chain's activity at once.

[edwarnicke]

@haiodo has also nice idea - create context that will exist per-chain. So we will have our usual context that is per-requers and new one per-chain. That allows us to cancel any chain's activity at once.

This sounds like a very very good idea.

[lobkovilya]

@edwarnicke please take a look, I rebased on latest changes and removed lazyInit

[edwarnicke]

Why are we zeroing out reported here rather than taking what is coming from INITIAL_STATE_TRANSFER?

And I don't see how we are correctly handling the re-request case for an INITIAL_STATE_TRANSFER that is missing some of the connections we think we have.

[lobkovilya]

If we write empty slice to grpc stream, we receive it like nil. AFAIK nil and empty map looks the same for grpc.

[lobkovilya]

At least I faced with panic here if the first INITIAL_STATE_TRANSFER is empty

[edwarnicke]

Oh... I see now :)

[edwarnicke]

Please move the test things from pkg/test/ to being in context in the monitor/ directory. Please for the moment put them into the monitor_test package in the monitor directory... until we actually need to reuse the test machinery, lets not export it.

[edwarnicke]

Have a look at #109 for an example of how to test without having to introduce a ton of new machinery :)