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

Refactor channel connectivity to avoid multiple spin loops #380

Merged
merged 9 commits into from
Feb 26, 2019

Conversation

rebello95
Copy link
Collaborator

The current Swift implementation of the gRPC channel's connectivity observer spins up a new ConnectivityObserver instance which then starts a new "spin loop" thread and continually runs, observing changes to the underlying gRPC Core channel's connectivity and piping those back through a callback closure. This means that there's a new spin loop spun up for each observer for each channel.

We can avoid having to spin up multiple spin loops for each observer (keeping only 0 or 1 per channel) by allowing a single ConnectivityObserver instance to pipe changes back to multiple callbacks.

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

Just a few suggestions.

Sources/SwiftGRPC/Core/Channel.swift Outdated Show resolved Hide resolved
Sources/SwiftGRPC/Core/Channel.swift Outdated Show resolved Hide resolved
Sources/SwiftGRPC/Core/Channel.swift Show resolved Hide resolved
Sources/SwiftGRPC/Core/Channel.swift Outdated Show resolved Hide resolved
The current Swift implementation of the gRPC channel's connectivity observer spins up a new ConnectivityObserver instance which then starts a new "spin loop" thread and continually runs, observing changes to the underlying gRPC Core channel's connectivity and piping those back through a callback closure. This means that there's a new spin loop spun up for each observer for each channel.

We can avoid having to spin up multiple spin loops for each observer (keeping only 0 or 1 per channel) by allowing a single ConnectivityObserver instance to pipe changes back to multiple callbacks.
@rebello95
Copy link
Collaborator Author

@MrMage thank you for the review! Updated with feedback addressed if you'd like to take another look

@rebello95
Copy link
Collaborator Author

Just noticed what looks like an existing issue where completionQueue.shutdown() would be called on deinit even if the observer was already shut down. Fixed that while I'm here

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

Thank you for the changes! Two minor changes left. When merging, please use "Squash and Merge" to keep the repo history clean.

Sources/SwiftGRPC/Core/ChannelConnectivityObserver.swift Outdated Show resolved Hide resolved
Sources/SwiftGRPC/Core/Channel.swift Outdated Show resolved Hide resolved
@rebello95 rebello95 merged commit 10aff09 into grpc:master Feb 26, 2019
@rebello95 rebello95 deleted the channel-spinloops branch February 26, 2019 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants