need a way to dispose of the http Client()
devoncarew opened this issue · 6 comments
When creating a GenerativeModel, you can optionally pass in an http Client. If you don't pass one in, an instance is created for you.
However, when I'm finished with the GenerativeModel, I don't have any way of disposing of the created http client (client.close()). I think we'll need a dispose or close on GenerativeModel (which may proxy through to ApiClient).
Hi, I've made a PR to fix this one. Can you please check it and let me know if this is okay?
Thanks :)
Omitting a close method was intentional - if the Http Client is constructed outside the class, I think it's better to not close it, since we don't know if it's in use elsewhere.
We use a temporary client for each request in the default case, so no need to close.
If we do add a close() method, I think we'd need to probably allow a choice of whether the client gets closed too.
Maybe we'd instead add a void Function() onClose callback and when you construct the model with your own client you can pass a callback that closes it.
I'm uneasy about adding a lifecycle hook which only sometimes does things though (with either design), it's very easy to use incorrectly and not realize it.
Omitting a close method was intentional - if the Http Client is constructed outside the class, I think it's better to not close it, since we don't know if it's in use elsewhere.
Yep!
We use a temporary client for each request in the default case, so no need to close.
Maybe that wasn't apparent from the code. One can confirm that by looking at:
http.post and the _withClient helper function
If we do add a close() method, I think we'd need to probably allow a choice of whether the client gets closed too. Maybe we'd instead add a
void Function() onClosecallback and when you construct the model with your own client you can pass a callback that closes it.I'm uneasy about adding a lifecycle hook which only sometimes does things though (with either design), it's very easy to use incorrectly and not realize it.
Lets not add a close() method. I suspect @devoncarew filed the bug because it isn't obvious that the Client is being closed in the default case.
Oops, sorry to act quick on this. I've removed the close method for now. Will keep my eyes on the project. Thanks for the guidelines.
because it isn't obvious that the Client is being closed in the default case.
Ah, yes, I thought that I'd need to close it myself in order to not have a CLI client hang around for 10-15 seconds waiting for that http connection to be GC'd. Sounds like we don't need a manual close.
@HeySreelal - thanks for the PR in any case!