fix(pubsub): close grpc streams on retry #10624
Conversation
follow-up as 66581c4 is not enough to plug all stream leaks
| } | ||
| // we are about to open a new stream: if necessary, make sure the previous one is closed | ||
| if s.close != nil { | ||
| s.close() |
There was a problem hiding this comment.
So if I'm reading this correctly, there's a desire to make sure the stream is closed, both here and in line 74 above. Why can't you call pullstream.cancel instead? Could you explain a bit more why an additional cancel func is necessary?
There was a problem hiding this comment.
Hi! So on L74 it's just to tidy up the context that's just been created rather than actually close the stream (since the stream creation would have errored out).
But an additional context and separate, corresponding cancel function are needed because the main pullStream ctx and cancel are meant to last for the whole lifecycle of pullStream, while the grpc streams that are leaked may have a shorter lifespan (precisely because of the retry, when you get back here on L115 after the initial stream creation), so need to be tied to a shorter-lived child context of the main pullStream one.
There was a problem hiding this comment.
Ack, yeah that makes sense. I'll approve and prioritize merging this week. Thanks for the contribution!
There was a problem hiding this comment.
Is there something I can help out with to get this into the next release? Suffering from the same memory leak as @itizir 🙏
Following up on #10094 and #10153: even after deploying with the fix that was merged, we continued seeing similar leaks in some conditions. We eventually realised that the reason was that the retry logic in
pullStreammeant grpc streams could still escape the closing mechanism that was added in #10153.Submitting this little PR with a suggestion on how to prevent such leaks.