Closed nullbtb closed 1 year ago
You actually went and did it - thanks!
Can you explain your patch a bit? On the surface it seems like a lot of code for what it's doing so I want to make sure I'm understanding it correctly.
I'll comment on the changes in more detail, just wanted to get an overview from you first.
You're welcome, seemed like an interesting challenge :). So the main culprit was the GRPC client's connection staying open. To address this its now doing a shutdown command on the client's channel. This causes the client to drop the connection. This was the main cause and reduced the lingering to like 10 seconds from like 4 minutes, but still wasn't resolved. I then noticed the auth client uses http client but did not close its connection. So I provided a method to be able to close that too. I also allowed a user to pass their own http client in initialize since the class already supported it.
The result is when you close the auth and firestore clients the program should end immediately. The other stuff is just an application default credential example and closing any potentially lingering streams which could cause memory leak issues. However there may be better ways to handle the stream closing. I think this should resolve the main issue but to be honest I didn't dive deeper into other aspects of the package.
Ok, I get it now. So essentially your PR is actually multiple patches:
Would you be ok with refactoring 2 and 3 as two separate PRs? I prefer to keep things focused.
As for 4, while I appreciate the motivation behind it, I think it detracts from its goal of being a minimal example of usage, so I'm not so sure about integrating it.
Kind of, so 4 really is just to demonstrate that you're supposed to close the Firestore grpc connection even if there is an exception in the code. It's not really about the memory leak concerns. I just wrapped it in a try catch finally so that it will always close the connection and allow the program to close properly. The only way to ensure that is in a finally clause. For example, if there is an error because the user does not have access to a collection it will throw an exception. However we still need to close the GRPC connection or it will just stay there waiting for a few minutes again.
Regarding 2, the main point is to be able to specify your own client so that you can manage the connection yourself outside of this package. I thought it was related because it relates to being able to close the connection yourself and allowing the program to exit. I can put this one as its own PR if you prefer though.
Regarding 3, this one is just an example using application default credentials. I can put it in its own PR.
On 4. I understood your intention, I just think the added try-catch-finally with indentation makes the code harder to read and more complex than I like in an example. I'm also not convinced that always closing the connection on exception is something we should be pushing hard on. I guess it's very implementation-specific.
On 2. and 3. yeah I get that both are somewhat related to your patch. In fact, now that they're in I guess there's no point in making separate patches for them, I feel like I'm just making extra work for you.
It's pretty late here in Germany so I won't be replying for a while. I'll test & approve tomorrow, and then we can push a new release to pub.dev.
Thanks!
Currently, there is no way to close some connections (#110) which causes a program to linger running until the connection drops. This PR provides methods to close the connections and allow the execution to complete.