Graceful exit #4

i-norden · 2019-01-28T20:43:52Z

elizabethengelman commented

2019-01-28 20:43:52 +00:00

(Migrated from github.com)

No description provided.

i-norden reviewed 2019-01-28 20:43:52 +00:00

TakaGoto (Migrated from github.com) reviewed 2019-01-28 20:43:52 +00:00

yaoandrew (Migrated from github.com) reviewed 2019-01-28 20:43:52 +00:00

rmulhol (Migrated from github.com) reviewed 2019-01-29 19:31:37 +00:00

rmulhol (Migrated from github.com) left a comment

Looking good! Left a few comments - mostly interested in the error condition in the for loop

statediff/service/service.go

rmulhol (Migrated from github.com) commented

2019-01-29 19:20:54 +00:00

Just curious - does this name provide additional functionality/would the body of the goroutine behave differently without it?

rmulhol (Migrated from github.com) commented

2019-01-29 19:26:02 +00:00

wondering if it makes sense to fail here - seems like parent block being nil should never happen, unless maybe we're at the genesis block?

statediff/testhelpers/mocks/blockchain.go

rmulhol (Migrated from github.com) commented

2019-01-29 19:27:56 +00:00

wouldn't this trigger subErr within two more passes through the for loop? are we expecting len(bc.ChainEvents) < 2)?

wouldn't this trigger `subErr` within two more passes through the for loop? are we expecting `len(bc.ChainEvents) < 2)`?

i-norden reviewed 2019-01-29 23:03:28 +00:00

statediff/service/service.go

i-norden commented

2019-01-29 23:03:27 +00:00

Looks like it is letting the select break exit the for loop, otherwise I think you could do something like

loop := true
for loop {
		select {
		//Notify chain event channel of events
		case chainEvent := <-chainEventCh:
			log.Debug("Event received from chainEventCh", "event", chainEvent)
			blocksCh <- chainEvent.Block
		//if node stopped
		case err := <-errCh:
			log.Debug("Error from chain event subscription, breaking loop.", "error", err)
			loop = false
		}
}

to get the same behavior

Looks like it is letting the select break exit the for loop, otherwise I think you could do something like ```go loop := true for loop { select { //Notify chain event channel of events case chainEvent := <-chainEventCh: log.Debug("Event received from chainEventCh", "event", chainEvent) blocksCh <- chainEvent.Block //if node stopped case err := <-errCh: log.Debug("Error from chain event subscription, breaking loop.", "error", err) loop = false } } ``` to get the same behavior

rmulhol (Migrated from github.com) reviewed 2019-01-29 23:10:25 +00:00

statediff/service/service.go

rmulhol (Migrated from github.com) commented

2019-01-29 23:10:25 +00:00

but wouldn't the existing code with just break (without a named loop) also do the same thing?

but wouldn't the existing code with just `break` (without a named loop) also do the same thing?

i-norden reviewed 2019-01-29 23:22:29 +00:00

statediff/service/service.go

i-norden commented

2019-01-29 23:22:29 +00:00

A plain break in the case block would only break out of the select statement afaik. It acts the same as the implicit breaks at the end of each case block or like breaks in a C switch-statement. Or maybe that's only with go switches and not selects?

elizabethengelman (Migrated from github.com) reviewed 2019-01-30 15:12:50 +00:00

statediff/service/service.go

elizabethengelman (Migrated from github.com) commented

2019-01-30 15:12:50 +00:00

That's a good point. I think that when we exit the geth command there's a chance that GetBlockByHash won't find the parent block, potentially because the connection to the db has already closed (?). But I think adding a error here is a good idea, because if it happens any other time, it would mean something is wrong.

That's a good point. I think that when we exit the geth command there's a chance that `GetBlockByHash` won't find the parent block, potentially because the connection to the db has already closed (?). But I think adding a error here is a good idea, because if it happens any other time, it would mean something is wrong.

elizabethengelman (Migrated from github.com) reviewed 2019-01-30 17:49:49 +00:00

statediff/service/service.go

elizabethengelman (Migrated from github.com) commented

2019-01-30 17:49:49 +00:00

My understanding is similar to what Ian said. If we just have a break without the named loop, it'll break from the current iteration through the select loop, so it would continue looping, and I don't think the go routine would be ended.

My understanding is similar to what Ian said. If we just have a `break` without the named loop, it'll break from the current iteration through the select loop, so it would continue looping, and I don't think the go routine would be ended.

elizabethengelman (Migrated from github.com) reviewed 2019-01-30 17:51:49 +00:00

statediff/service/service.go

elizabethengelman (Migrated from github.com) commented

2019-01-30 17:51:49 +00:00

It looks like named loops are used in several other places in the codebase, but I'd also be happy to change it to the implementation that @i-norden mentioned above, if that makes more sense.

elizabethengelman (Migrated from github.com) reviewed 2019-01-30 19:04:01 +00:00

statediff/service/service.go

elizabethengelman (Migrated from github.com) commented

2019-01-30 19:04:01 +00:00

Actually the more that I think about it, I'm not sure how/where an error would go and bubble up, since we're running the loop in a goroutine.

I wonder if we could follow a similar pattern as the previous for loop, using a named loop. So that if parentBlock == nil it will break from the for loop, and end the service.

Actually the more that I think about it, I'm not sure how/where an error would go and bubble up, since we're running the loop in a goroutine. I wonder if we could follow a similar pattern as the previous for loop, using a named loop. So that if `parentBlock == nil` it will break from the for loop, and end the service.

elizabethengelman (Migrated from github.com) reviewed 2019-01-30 23:15:38 +00:00

statediff/service/service.go

elizabethengelman (Migrated from github.com) commented

2019-01-30 23:15:38 +00:00

just a note for reference, the channel passed into the Subscription should "ample buffer space to avoid blocking other subscribers"

just a note for reference, the channel passed into the Subscription should "[ample buffer space to avoid blocking other subscribers](https://github.com/vulcanize/go-ethereum/blob/master/event/feed.go#L71)"

elizabethengelman (Migrated from github.com) reviewed 2019-01-30 23:24:12 +00:00

statediff/testhelpers/mocks/blockchain.go

elizabethengelman (Migrated from github.com) commented

2019-01-30 23:24:11 +00:00

For the first test (testErrorInChainEventLoop) I think that this should return the subErr when the for loop gets to the third event - since the counter starts at 0 (for the first event), and then is incremented to 1 (for the second event). Not sure if this is the best way to illustrate this though, it's a bit confusing I think, so if you have any other ideas, I'd be happy to change it!

For the first test (`testErrorInChainEventLoop`) I think that this should return the `subErr` when the for loop gets to the third event - since the counter starts at 0 (for the first event), and then is incremented to 1 (for the second event). Not sure if this is the best way to illustrate this though, it's a bit confusing I think, so if you have any other ideas, I'd be happy to change it!

rmulhol (Migrated from github.com) reviewed 2019-01-31 00:56:21 +00:00

statediff/service/service.go

rmulhol (Migrated from github.com) commented

2019-01-31 00:56:21 +00:00

I'm totally good with the named loop, just missed that omitting the name would only break out of the select statement (rather than the for loop, as desired)

I'm totally good with the named loop, just missed that omitting the name would only break out of the `select` statement (rather than the `for` loop, as desired)