[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: |
|
||||||||
| 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: Co-authored-by: Benjamin Rewis <32186188+benjirewis@users.noreply.github.com> | ||
| 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 | ||
| 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 ] | ||
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 | ||
| 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 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: If it’s active requests:
To enforce an active timeout of 30 seconds. Let me know what sounds better/what might solve the issue you’re seeing. |