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

Unsubscribe all connectivity callbacks on Channel.close #19030

Merged
merged 1 commit into from
May 17, 2019

Conversation

mehrdada
Copy link
Member

Fixes #18995

There could be a better fix available, but seems like the code should be reworked more fundamentally anyway so this simple fix could be fine for now

@mehrdada
Copy link
Member Author

cc @lidizheng

Copy link
Contributor

@lidizheng lidizheng left a comment

Choose a reason for hiding this comment

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

Can you try to add a small unit test for this behavior, to prevent regression?

@mehrdada
Copy link
Member Author

@lidizheng it's hard to test against it without refactoring the logic entirely (it'd be easier to fix the entire logic than to test against it)--the reason being the symptom (i.e. the exception) is thrown on a different thread.

@lidizheng
Copy link
Contributor

I agree the text logic probably is more complicate than the fix itself. But it shouldn’t be a challenging one. You can create the callback with the access to the unittest object.

I will label this as help wanted, so if someone have bandwidth can work on it.

@lidizheng lidizheng added disposition/help wanted Maintainers do not have enough resources to allocate to this at the moment. Help is appreciated! lang/Python release notes: yes Indicates if PR needs to be in release notes labels May 15, 2019
@lidizheng lidizheng assigned lidizheng and unassigned lidizheng May 15, 2019
@mehrdada
Copy link
Member Author

There's no test for any of these in Channel.close (hence the mess in the first place). If you feel this is not worthwhile progress to fix customer pains short term, then fine. Otherwise, I recommend merging and filing a "help wanted" issue for adding the test.

@lidizheng lidizheng merged commit ebfed6a into grpc:master May 17, 2019
@mehrdada mehrdada deleted the unsubscribe_all branch May 23, 2019 21:41
@lock lock bot locked as resolved and limited conversation to collaborators Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
disposition/help wanted Maintainers do not have enough resources to allocate to this at the moment. Help is appreciated! lang/Python release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connectivity callback prints exception trace in background thread
4 participants