[GODRIVER-2050] Be able to supply http.Transport for OCSP Created: 15/Jun/21  Updated: 28/Oct/23  Resolved: 31/Oct/22

Status: Closed
Project: Go Driver
Component/s: Networking
Affects Version/s: None
Fix Version/s: 1.11.0

Type: Improvement Priority: Minor - P4
Reporter: Eric Daniels Assignee: Matt Dale
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Depends
is depended on by GODRIVER-2623 Use a clone of the http.DefaultTransp... Closed

 Description   

Right now, http.DefaultClient is used which can leave lingering goroutines until CloseIdleConnections is called. This is not desirable in multi-tenant applications or daemonized applications managed by the same go process. Preferably, a ClientContext like method that supplies an http.Client or http.Transport could be created for this purpose; a new option would also work as well, but may be a little bloaty.



 Comments   
Comment by Githook User [ 31/Oct/22 ]

Author:

{'name': 'Eric Daniels', 'email': 'eric@erdaniels.com', 'username': 'edaniels'}

Message: GODRIVER-2050 - Add settable http.Clients and gracefully close provided defaults (#1093)

Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com>
Co-authored-by: Matt Dale <9760375+matthewdale@users.noreply.github.com>
Branch: master
https://github.com/mongodb/mongo-go-driver/commit/4a22ce6d689a699f0ba2f59cda7bc9a6a707935c

Comment by Benji Rewis (Inactive) [ 17/Oct/22 ]

Thanks so much eric@erdaniels.com ! Taking a look now (just authorized the evergreen patch).

Comment by Eric Daniels [ 17/Oct/22 ]

np! Just put up a PR. It was fairly trivial to add and made easier with options.

Comment by Benji Rewis (Inactive) [ 14/Oct/22 ]

Sounds great  thanks, eric@erdaniels.com ! 

Comment by Eric Daniels [ 14/Oct/22 ]

Right, that's why having both as options would be ideal. Worst case I can maintain a fork.

Comment by Benji Rewis (Inactive) [ 14/Oct/22 ]

I'd also like to experiment with passing an option for the same http.Client since it's fairly common to provide a more customized client than the default provided by the standard library.

That sounds correct to me, but I think the bulk of users will not think to provide a custom http.Client and call CloseIdleConnections on it. It would probably be more seamless to call CloseIdleConnections from http.Client and warning that we may be slightly more hesitant to accept a PR that adds a custom http.Client option for that reason.

Comment by Eric Daniels [ 14/Oct/22 ]

Yeah that sounds good. I'd also like to experiment with passing an option for the same http.Client since it's fairly common to provide a more customized client than the default provided by the standard library. I'll try to take a pass pretty soon.

Comment by Benji Rewis (Inactive) [ 14/Oct/22 ]

Hello again, eric@erdaniels.com. The team probably won't have time to do this ourselves in the near future, so you're totally welcome to submit a PR.

Rereading this: sounds like what you want is to bound the lifetime of resources created by creating and using a mongo.Client to the lifetime of the mongo.Client, which is definitely reasonable. That also seems like a improvement for all users, so we're thinking a better change might be to create an http.Client with a mongo.Client, use it for all OCSP queries, and call CloseIdleConnections on it in monog.Client.Disconnect. What do you think?

Comment by Eric Daniels [ 13/Oct/22 ]

So it's been about a year. Should I submit a PR instead?

Comment by Eric Daniels [ 28/Oct/21 ]

That works for me.

Comment by Benji Rewis (Inactive) [ 28/Oct/21 ]

Ah very interesting. Having complete control over the client (i.e. allowing a custom http.Client or http.Transport in VerifyOptions) used to contact OCSP responders makes sense to me and doesn't seem like a big change. That's certainly an option here. It sounds like it'd be even cleaner if something like mongo.Client.Disconnect also called CloseIdleConnections on that http.Client. That way, closing the Mongo client would also shut down those idle OCSP connections. That change seems slightly more complex (would have to store the OCSP http.Client on mongo.Client) but still feasible 

In any case, if this is not super high priority for you (do you have a workaround?), I'll remove the fix version on this ticket and leave it in "Open" for now.

Comment by Eric Daniels [ 28/Oct/21 ]

Hey Benji! No worries . They are idle goroutines and in a system where thousands can be created for whatever reason, it's nice to have complete control/knowledge about what's out there and that they are all closed down. Calling CloseIdleConnections is definitely a choice but feels a little clunky to me to have to combine it with closing down the mongo client which I would expect would clean up completely after itself, something it has historically strived to do while I had some involvement on the driver.

This is truly not high priority so don't worry about getting back to this quickly!

Comment by Benji Rewis (Inactive) [ 26/Oct/21 ]

Hello eric@erdaniels.com!

Apologies for the crazy delay in response; we’ve had some higher priority work and just got around to this ticket

I assume you’re referring to the DefaultClient used here when contacting the responders for OCSP verification. I’m curious which goroutines are “lingering” for you. Are they idle goroutines? Or, are they active requests that haven’t yet made it to the OCSP responders?

If it’s idle connections:
DefaultClient uses DefaultTransport as its RoundTripper, which means that the IdleConnTimeout will be 90 seconds. If you’re finding that the 90 seconds isn’t a long enough idle timeout, we could allow specifying a custom http.Client or http.Transport (as you suggested) so you could either set a lower IdleConnTimeout or manually call CloseIdleConnections.

If it’s active requests:
DefaultClient has no regular Timeout, which means that any request made through DefaultClient will never timeout while active. CloseIdleConnections would not be able to close goroutines with non-idle connections, so we’d need to set a Timeout on the http.Client used. We could do that pretty easily by changing this line to:

client := &http.Client{Timeout: 30 * time.Second}
httpResponse, err := client.Do(request)

To enforce an active timeout of 30 seconds.

Let me know what sounds better/what might solve the issue you’re seeing.

Generated at Thu Feb 08 08:37:43 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.