Add a new connectionFactory property to HttpClient

[brianquinlan]

Change

I propose that we add a new connectionFactory property to HttpClient with a signature like:

void connectionFactory=(
  Future<ConnectionTask<Socket>> f(
    Uri url, String? proxyHost, int? proxyPort)?
)

This would allow users to customize socket creation (e.g. for communication with dockerd/snapd over Unix domain sockets). Full details in the design doc.

The non-breaking alternative would be to make connectionFactory an argument to the HttpClient constructor.

Rationale

The HttpClient API already has multiple attributes that accept functions e.g.

void findProxy=(
  String f(Uri url)?)

void badCertificateCallback=(
  bool callback(
    X509Certificate cert,
    String host,
    int port
  )?
)

and implementing this as a constructor argument would be inconsistent with the rest of the API. Also, parameterizing all future functionality as constructor arguments does not seem scalable.

Impact

All classes that implements HttpClient (without extends Mock or equivalent noSuchMethod implementation) will need to be updated.

I suspect that there are not many implementations of HttpClient outside of mocks.

Mitigation

Users must implement the connectionFactory property.

[kevmoo]

Why not add another, optional, constructor param that takes the factory? Do we expect users to want to change this over the lifetime of an instance?

CC @natebosch

[brianquinlan]

@kevmoo I suggested that in my description i.e. the non-breaking alternative would be to make 'connectionFactory' an argument to the HttpClient constructor.

I'm fine with doing that but, if we always use that reasoning, all future parameters will be added as constructor arguments. That will:

1. make for inconsistent APIs e.g. why is findProxy a property and connectionFactory a constructor argument?
2. make constructors grow larger as an API evolves

If we decide that we can't make breaking changes by adding properties, then we should probably avoid using them, to the extent possible, when design new APIs because they will look odd when future functionality is added as constructor arguments.

[kevmoo]

Lots of named, optional parameters are fine, IMHO. Look at any Flutter API.

I think we should avoid foolish inconsistencies here. Having these be in the constructor (we don't even have to expose the field then) is cleaner and more idiomatic IMHO.

...UNLESS we have a reasonable use case where we want to be able to change the function during lifetime.

Curious what @natebosch @lrhn and/or @jakemac53 have to say

[brianquinlan]

@kevmoo fair enough

I don't think that users will need to be able to change the function - and, if they really need to, they can always use a conditional in the function that they assign.

[brianquinlan]

One interesting complication of adding a new HttpClient constructor argument is do I also add a new argument to HttpOverrides.createHttpClient?

Because, if I do, that will also break code.

If I don't then what's the rationale for having a securityContext argument but not connectionFactory?

[natebosch]

My initial inclination is to add this to the constructor, but I agree that it's a problem to have an inconsistency with HttpOverrides.createHttpClient.

[brianquinlan]

At least in Google code, there is at least an order of magnitude more definitions of createHttpClient than implements HttpClient (without extends Mock or equivalent noSuchMethod implementation).

[brianquinlan]

So how do we come to a decision here? I think that we have discussed 3 possibilities:

1. add a new connectionFactory property to HttpClient - breaks all code that implements HttpClient (without extends Mock or equivalent noSuchMethod implementation): there are not many such cases at Google
2. add a new connectionFactory argument to the HttpClient constructor and to HttpOverrides.createHttpClient - breaks all code that implements createHttpClient: there are many such cases at Google
3. add a new connectionFactory argument to the HttpClient but do not modify HttpOverrides - very compatible but makes the createHttpClient inconsistent with the actual HttpClient constructor

I prefer (1) because it makes for a consistent API with a reasonably low probability of significant breakage.

But I'm happy to do any of these.

[brianquinlan]

@natebosch @kevmoo Could you provide your opinions regarding #47887 (comment) ?

[natebosch]

@lrhn - WDYT?

I'd lean towards 3, but none of the options are very attractive.

[brianquinlan]

If we go with (3), will we never update HttpOverrides.createHttpClient as we add new HttpClient constructor arguments? So essentially the HttpOverrides API will match the behavior of HttpClient in 2017 forever?

I'm asking because I'm trying to understand our overall strategy regarding API evolution.

[natebosch]

I'm asking because I'm trying to understand our overall strategy regarding API evolution.

We're all trying to figure out our strategy for API evolution.

[lrhn]

How about making it a completely new constructor, HttpClient.withConnectionFactory, and add a new method to HttpOverrides, createHttpClientWithConnectionFactory?

(Names need more work!)

Or what if the connection factory itself was placed in the HttpOverrides too, so you just created a new override from the old override and added the createConnection override there before doing HttpClient(). It seems to me that it's an override-like functionality to change how connections are created.

[brianquinlan]

@lrhn

Regarding your first idea: I think that would work. Theoretically, adding a createHttpClientWithConnectionFactory method to HttpOverrides could break some code but I see only one case of implements HttpOverrides in Google code.

Regarding your second idea, so the change would look like (not tested):

abstract class HttpOverrides {
  ...
  static R runZoned<R>(R Function() body,
      {HttpClient Function(SecurityContext?)? createHttpClient,
      String Function(Uri uri, Map<String, String>? environment)?
          findProxyFromEnvironment,
       Future<ConnectionTask<Socket>>Function(HttpClient client, Uri url, String? proxyHost, int? proxyPort, SecurityContext? context) createConnection?
}) ...

  Future<ConnectionTask<Socket>> createConnection(HttpClient client, Uri url, String? proxyHost, int? proxyPort, SecurityContext? context) {
    if (proxyHost != null) {
      return SecureSocket.startConnect(proxyHost, proxyPort)
    }
    if (url.isScheme("https")) {
      return SecureSocket.startConnect(host, port,
              context: context, onBadCertificate: client.badCertificateCallback)
    }
    return SecureSocket.startConnect(url.host, url.port)
  }

  HttpClient createHttpClient(SecurityContext? context) {
    return _HttpClient(context, createConnection); // Pass in the connection factory
  }
}

Someone who wanted to do something like support unix domain sockets:

Future<ConnectionTask<Socket>> createUnixConnection(HttpClient client, Uri uri, String? proxyHost, int? proxyPort, SecurityContext? context) {
  if (uri.scheme == 'unix') {
    assert(proxyHost == null);
    assert(proxyPort == null);
    var address = InternetAddress(Uri.decodeComponent(uri.host),
        type: InternetAddressType.unix);
    return Socket.startConnect(address, 0);
  } else {
     // Same as from `createConnection`
  }
}

void main() {
  HttpOverrides.runZoned(() {
    final httpClient = HttpClient()
    httpClient.findProxy = (uri) {
      if (uri.scheme == 'unix') {
         // Proxy settings are not meaningful for Unix domain sockets.
         return 'DIRECT';
      } else {
         return HttpClient.findProxyFromEnvironment(uri);
      }
    };
    httpClient
      .getUrl(Uri(
          scheme: "unix",
          host: Uri.encodeComponent("/var/run/docker.sock"),
          path: "/v1.41/containers/create"));
  }, createConnection: createUnixConnection);
}

One issue here: if the user wanted to mock this behavior for tests, how would they do this since the production code is already using HttpOverrides.

[brianquinlan]

After more thought and discussion around this, I strongly favor (1) i.e.

1. add a new connectionFactory property to HttpClient - breaks all code that implements HttpClient (without extends Mock or equivalent noSuchMethod implementation): there are not many such cases at Google

Everything else seems like it makes a bigger mess.

[devoncarew]

@brianquinlan, it sounds like we're reviewing the proposed change in #47887 (comment); i.e., option 1 / adding a new connectionFactory property to HttpClient.

@vsm, @Hixie, and @grouma - can you review and sign off for Dart, Flutter, and AngularDart respectively? Thanks!

[grouma]

I couldn't find any usages of implements HttpClient in the Angular code base so this should not have any impact.

[Hixie]

I have no objection.

I would love to see this documented in the API docs with sample code and so on to show how to use it.

[vsmenon]

Do we have summary on level of impact on internal and external code of this breaking change?

[brianquinlan]

@vsmenon For Google code, there are a few places that implement HttpClient without noSuchMethod or equivalent.

I checked some likely-to-be-affected external packages e.g. package:http, package:http2 but didn't find any that would be broken.

[vsmenon]

lgtm

[devoncarew]

@brianquinlan - you're good to go with this change.

[brianquinlan]

Fixed in a0aeed9

[fortuna]

The connectionFactory functionality with proxies is poorly documented.
What should I actually do with the proxy host and port?
Do I:

1. Return a connection to the proxy, and the HttpClient automatically does the proxy client logic to connect to the target
2. Or do I return a connection to the desired target, in which case I need to implement both the connection to the proxy, and the connection to the target via the proxy?

If #2, then how do I know what type of proxy we are using? My understanding is that for PROXY, it could be HTTP or HTTPS, based on the protocol of the URL, but doesn't findProxy also allow HTTP, HTTPS or SOCKS specification? In that case, how would I know the type of the proxy?

[brianquinlan]

@fortuna Could you file a new issue asking for these documentation fixes?