Project

General

Profile

Actions

Idea #9005

closed

[SDKs] Go SDK's arvadosclient and keepclient should share http.Client objects by default

Added by Tom Clegg almost 8 years ago. Updated almost 7 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
SDKs
Target version:
Story points:
1.0

Description

Background

As an example, keep-web currently
  • gets an arvadosclient.ArvadosClient from an arvadosclient.ClientPool
  • configures it with the current token
  • passes it to keepclient.MakeKeepClient() in order to get a keepclient.Client

(The public APIs for creating a keepclient.Client all involve passing in an arvadosclient.ArvadosClient, so we can't just make a pool of keepclient.Clients and assume they won't leak credentials across requests.)

At this point, keepclient.MakeKeepClient() creates a new http.Client object. This means
  • Consecutive keep-web requests cannot reuse http connections to keep services
  • (Currently) the HTTP connections to keep services are left open anyway, because we don't tell the http.Client that we won't be reusing it

In general, sharing an http.Client across requests is beneficial because it reduces TCP and TLS handshaking.

Proposed improvements

keepclient should use kc.Arvados.Client instead of its own Client field unless the caller specifies otherwise.
  • keepclient.New() should leave the Client field set to nil by default.
  • keepclient methods that use an http client should get it from a private httpClient() method that works along the lines of the one in source:sdk/go/arvados/client.go: use the KeepClient field if non-nil, otherwise the Arvados one.

(This change alone should reduce the proliferation of http.Client objects significantly.)

Also, arvadosclient.MakeArvadosClient should reuse the same two http.Client objects for all new ArvadosClient objects -- e.g., arvadosclient.DefaultHTTPClient when ApiInsecure is false, arvadosclient.DefaultInsecureHTTPClient when true. The caller can override it later if desired.

arvadosclient.DefaultHTTPClient should default to http.DefaultClient. (keepclient modifies client timeouts and keepalive settings according to proxy/non-proxy config, and we shouldn't muck with the http package globals this way.)

(This change should reduce the proliferation of http.Client objects further, especially in programs that don't use an arvados client pool.)


Subtasks 4 (0 open4 closed)

Task #11773: use RefreshKeepServers in keep-webResolvedTom Clegg05/29/2017Actions
Task #11761: Review 9005-keep-http-clientResolvedPeter Amstutz05/31/2017Actions
Task #11772: Share http.Client and http.Transport by defaultResolvedTom Clegg05/29/2017Actions
Task #11795: Review 9005-share-discoveryResolvedTom Clegg05/29/2017Actions
Actions

Also available in: Atom PDF