Skip to content

Prevent another exception spam in _ChannelCallState.__del__#23201

Merged
lidizheng merged 1 commit intogrpc:masterfrom
lidizheng:fix-del-2
Jun 12, 2020
Merged

Prevent another exception spam in _ChannelCallState.__del__#23201
lidizheng merged 1 commit intogrpc:masterfrom
lidizheng:fix-del-2

Conversation

@lidizheng
Copy link
Copy Markdown
Contributor

@lidizheng lidizheng commented Jun 12, 2020

As title.

This issue originated from google3 (b/158863508), where the GC might behave slightly different than OSS. In theory, this function should hold reference against cygrpc and cygrpc.StatusCode. It's surprising that the dependencies of _ChannelCallState.__del__ being deallocated earlier than itself.

Overall, Python's GC is working fine in the runtime. Indeterministic behaviors only observed during interpreter shutdowns. The google3 issue is no exception. I have manually tested that this corner case is fixed by this PR.

If there is another spam, we should consider suppress the exception instead of patching up corner cases. (Error suppression considered harmful for project's debuggability.)

@lidizheng lidizheng added lang/Python release notes: no Indicates if PR should not be in release notes labels Jun 12, 2020
@lidizheng lidizheng changed the title Suppress the exception generated by _ChannelCallState __del__ Prevent another exception spam in _ChannelCallState.__del__ Jun 12, 2020
@lidizheng lidizheng marked this pull request as ready for review June 12, 2020 20:34
@lidizheng lidizheng requested a review from gnossen June 12, 2020 20:34
Copy link
Copy Markdown
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

And this is one of the major problems with __del__. You don't get many guarantees about what state your object (or the process) is in when it's called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lang/Python release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants