Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't retry when context cancelled. #204

Merged
merged 1 commit into from
Feb 13, 2020
Merged

Don't retry when context cancelled. #204

merged 1 commit into from
Feb 13, 2020

Conversation

lkysow
Copy link
Member

@lkysow lkysow commented Feb 12, 2020

Previously, when the context was cancelled we would enter the error
handling clause and the loop would be re-executed. Then the
backoff.Retry would exit immediately because the context was cancelled
and we'd run in a tight loop until the program was killed.

With this change, when we exit the backoff.Retry with an error, we check
if the context is done. If so, we exit immediately.

This functionality already existed in syncer.go:

		// Get all services with tags.
		var serviceMap map[string][]string
		var meta *api.QueryMeta
		err := backoff.Retry(func() error {
			var err error
			serviceMap, meta, err = s.Client.Catalog().Services(opts)
			return err
		}, backoff.WithContext(backoff.NewExponentialBackOff(), ctx))

		// If the context is ended, then we end
		if ctx.Err() != nil {
			return
		}

@@ -176,10 +176,10 @@ func (s *ConsulSyncer) watchReapableServices(ctx context.Context) {
}, backoff.WithContext(backoff.NewExponentialBackOff(), ctx))
if err != nil {
s.Log.Warn("error querying services, will retry", "err", err)
continue
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, this continue would have us immediately restarting the for loop. Instead we should fall down into the select statement where we retry after a defined period of time

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I added a question inline, but nothing that should block this. You've mentioned that this is a bug; is it a bug that we could add tests for?

catalog/to-consul/syncer.go Outdated Show resolved Hide resolved
@lkysow
Copy link
Member Author

lkysow commented Feb 12, 2020

Looks good! I added a question inline, but nothing that should block this. You've mentioned that this is a bug; is it a bug that we could add tests for?

It's more of a bug that we see during our unit tests because we're stopping the app many times and so it continues to loop.

Maybe we can add a test, let me think about it.

@lkysow
Copy link
Member Author

lkysow commented Feb 12, 2020

@ishustava I added a test

Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, Luke! Thanks for adding the test!

catalog/to-consul/syncer_test.go Show resolved Hide resolved
Previously, when the context was cancelled we would enter the error
handling clause and the loop would be re-executed. Then the
backoff.Retry would exit immediately because the context was cancelled
and we'd run in a tight loop until the program was killed.

With this change, when we exit the backoff.Retry with an error, we check
if the context is done. If so, we exit immediately.
@lkysow lkysow merged commit 4035629 into namespace Feb 13, 2020
@lkysow lkysow deleted the ctx-retry branch February 13, 2020 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants