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

feat: use contexts for the sync service #456

Merged
merged 3 commits into from Feb 7, 2020
Merged

Conversation

@Stebalien
Copy link
Contributor

Stebalien commented Feb 3, 2020

This patch also simplifies several parts of the sync service given that we can now just cancel contexts to, e.g., cancel subscriptions.

@Stebalien Stebalien force-pushed the feat/context-everywhere branch from 6af6b9c to a12a09f Feb 3, 2020
sdk/sync/common.go Show resolved Hide resolved
sdk/sync/common.go Show resolved Hide resolved
sdk/sync/subscription.go Show resolved Hide resolved
sdk/sync/subscription.go Show resolved Hide resolved
sdk/sync/watch.go Show resolved Hide resolved
sdk/sync/watch.go Outdated Show resolved Hide resolved
sdk/sync/watch.go Show resolved Hide resolved
@Stebalien Stebalien requested a review from nonsense Feb 3, 2020
@@ -92,22 +92,16 @@ func Run(runnerName string, resultPath string) error {
// Now let the test case tell us how to configure the network.
subtree := sync.NetworkSubtree(instance.Hostname)
networkChanges := make(chan *sync.NetworkConfig, 16)
closeSub, err := instance.Watcher.Subscribe(subtree, networkChanges)
if err != nil {
if _, err := instance.Watcher.Subscribe(ctx, subtree, networkChanges); err != nil {

This comment has been minimized.

Copy link
@nonsense

nonsense Feb 4, 2020

Member

Shouldn't we have a closeSub in a defer statement?

This comment has been minimized.

Copy link
@Stebalien

Stebalien Feb 4, 2020

Author Contributor

Doesn't matter now. The subscription will be canceled when the context is canceled.

We only return that function (a) for convenience and (b) to indicate to the user that the subscription needs to be released somehow.

Thoughts? Would it be cleaner to remove it? Should we just always call it?

This comment has been minimized.

Copy link
@nonsense

nonsense Feb 4, 2020

Member

I only raised this, because I saw we used to call it - previously cancel was called when this function returns, and now it is called when the context is canceled and I am not sure if this is the same time, but it looks like it is the same time, so as you said - doesn't matter.

@nonsense

This comment has been minimized.

Copy link
Member

nonsense commented Feb 4, 2020

LGTM. I also tested this and we don't appear to have broken anything, and it is mostly equivalent to before.

@Stebalien

This comment has been minimized.

Copy link
Contributor Author

Stebalien commented Feb 4, 2020

Hm. We're not running the sync service tests in CI.

@Stebalien Stebalien force-pushed the feat/context-everywhere branch from a12a09f to cdb4071 Feb 4, 2020
@Stebalien Stebalien requested a review from nonsense Feb 4, 2020
@Stebalien Stebalien force-pushed the feat/context-everywhere branch from cdb4071 to 17a21a4 Feb 4, 2020
@Stebalien Stebalien dismissed nonsense’s stale review Feb 5, 2020

new changes

@nonsense

This comment has been minimized.

Copy link
Member

nonsense commented Feb 6, 2020

@Stebalien sorry for the conflicts. I was under the impression we had merged this! Will review the new changes now.

sdk/sync/subscription.go Outdated Show resolved Hide resolved
@nonsense nonsense requested a review from raulk Feb 7, 2020
@nonsense

This comment has been minimized.

Copy link
Member

nonsense commented Feb 7, 2020

Adding @raulk as well as original author of this code. The code in this service is a bit complex for my taste, and I am still trying to understand all the inner workings of this service and how the various contexts and cancels fit together.

@raulk

This comment has been minimized.

Copy link
Member

raulk commented Feb 7, 2020

@nonsense you should've seen it before we switched to redis pubsub 😆

@raulk
raulk approved these changes Feb 7, 2020
Copy link
Member

raulk left a comment

Had a quick skim, and this LGTM. Can't believe I overlooked using contexts to control lifecycles since the beginning 🤷‍♂ Thanks for undertaking this refactor, @Stebalien!

plans/dht/test/common.go Show resolved Hide resolved
plans/dht/test/common.go Show resolved Hide resolved
sdk/sync/common.go Show resolved Hide resolved
sdk/sync/common.go Show resolved Hide resolved
Copy link
Member

raulk left a comment

My only concern is that the context passed to the constructors {Must,New}{Watcher/Writer/WatcherWriter} and redisClient is very misleading, as it doesn't govern the lifecycle of the components that are bring constructed. Not sure how to best warn about that. I guess in godocs? Can't think of a different API shape that would be ergonomic enough to avoid the confusion in the first place.

sdk/sync/common.go Show resolved Hide resolved
@Stebalien

This comment has been minimized.

Copy link
Contributor Author

Stebalien commented Feb 7, 2020

Let's get #499 merged first. This needs to be tested in CI.

@Stebalien

This comment has been minimized.

Copy link
Contributor Author

Stebalien commented Feb 7, 2020

My only concern is that the context passed to the constructors {Must,New}{Watcher/Writer/WatcherWriter} and redisClient is very misleading, as it doesn't govern the lifecycle of the components that are bring constructed. Not sure how to best warn about that. I guess in godocs? Can't think of a different API shape that would be ergonomic enough to avoid the confusion in the first place.

Note: this is mostly confusing because of how we abuse contexts. Contexts are supposed to govern request lifetime, not service lifetime.

I've documented it now.

@Stebalien Stebalien force-pushed the feat/context-everywhere branch from 17a21a4 to b003853 Feb 7, 2020
Stebalien added 3 commits Feb 3, 2020
This patch also simplifies several parts of the sync service given that we can now just cancel contexts to, e.g., cancel subscriptions.
@Stebalien Stebalien force-pushed the feat/context-everywhere branch from 0f8bf46 to 37e46a5 Feb 7, 2020
@Stebalien Stebalien merged commit 88afb56 into master Feb 7, 2020
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 82.08% of diff hit (target 11.82%)
Details
codecov/project Absolute coverage decreased by -0.48% but relative coverage increased by +70.25% compared to 929883f
Details
@Stebalien Stebalien deleted the feat/context-everywhere branch Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.