Skip to content

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.

@rebello95 rebello95 requested review from MrMage and timburks February 25, 2019 15:12
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.

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.

@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