Repository navigation
BackgroundConsumer destruction issues #788
Description
We recently merged a PR that clears some variables that were causing a memory leak in the bidirectional streaming classes. It seems like after getting that fix in, it exposed a deeper issue: the background thread isn't exiting as expected.
Quick Fix
When this happens, it's leading to a null pointer exception here, now that the _on_response can be empty. So as a quick fix, we should add a None check there
Deeper Issue
The root issue is that the BackgroundConsumer's background thread is getting stuck calling _on_response while being torn down. In the case of Firestore, it looks like the deadlock is the fault of the Watch class, which tries to re-enter a lock. But I think we should consider changing the BackgroundConsumer to close quicker, without doing that final on_response call at all. Is it really necessary to call that callback while being closed?
More Context
googleapis/python-firestore#985 (comment)
Let me know if you want me to submit a PR
Activity
parthea commentedon Feb 13, 2025
Yes, please go ahead and open a PR
vchudnov-g commentedon Apr 10, 2025
@daniel-sanche Could you file a GitHub issue for the "Deeper Issue", so we remember to come back to it?
daniel-sanche commentedon Apr 10, 2025
Actually looking at it again, I think most of the blame falls on firestore's callback implementation.
I think this should be safe to close with the fixes in place