🐛 Make cached connections respect redirections (#2353)

The connection manager now established the transport with the given redirection record rather than reusing the previous transport.

plugins/http2_adapter/CHANGELOG.md

@@ -1,8 +1,11 @@
 # CHANGELOG
 
+**Before you upgrade: Breaking changes might happen in major and minor versions of packages.<br/>
+See the [Migration Guide][] for the complete breaking changes list.**
+
 ## Unreleased
 
-*None.*
+- Make cached connections respect redirections.
 
 ## 2.5.3
 
@@ -80,3 +83,5 @@
 ## 0.0.2 - 2019.9.17
 
 - A Dio HttpAdapter which support Http/2.0.
+
+[Migration Guide]: doc/migration_guide.md

plugins/http2_adapter/doc/migration_guide.md

@@ -0,0 +1,31 @@
+# Migration Guide
+
+This document gathered all breaking changes and migrations requirement between versions.
+
+<!--
+When new content need to be added to the migration guide, make sure they're following the format:
+1. Add a version in the *Breaking versions* section, with a version anchor.
+2. Use *Summary* and *Details* to introduce the migration.
+-->
+
+## Breaking versions
+
+- [2.6.0](#260)
+
+## 2.6.0
+
+### Summary
+
+- `ConnectionManager.getConnection` now requires redirection records as a parameter.
+
+### Details
+
+#### `ConnectionManager.getConnection`
+
+```diff
+ /// Get the connection(may reuse) for each request.
+ Future<ClientTransportConnection> getConnection(
+   RequestOptions options,
++  List<RedirectRecord> redirects,
+ );
+```

plugins/http2_adapter/lib/src/connection_manager.dart

@@ -18,7 +18,10 @@ abstract class ConnectionManager {
       );
 
   /// Get the connection(may reuse) for each request.
-  Future<ClientTransportConnection> getConnection(RequestOptions options);
+  Future<ClientTransportConnection> getConnection(
+    RequestOptions options,
+    List<RedirectRecord> redirects,
+  );
 
   void removeConnection(ClientTransportConnection transport);
 

plugins/http2_adapter/lib/src/connection_manager_imp.dart

@@ -34,47 +34,60 @@ class _ConnectionManager implements ConnectionManager {
   @override
   Future<ClientTransportConnection> getConnection(
     RequestOptions options,
+    List<RedirectRecord> redirects,
   ) async {
     if (_closed) {
       throw Exception(
         "Can't establish connection after [ConnectionManager] closed!",
       );
     }
-    final uri = options.uri;
-    final domain = '${uri.host}:${uri.port}';
-    _ClientTransportConnectionState? transportState = _transportsMap[domain];
+    Uri uri = options.uri;
+    if (redirects.isNotEmpty) {
+      uri = Http2Adapter.resolveRedirectUri(uri, redirects.last.location);
+    }
+    // Identify whether the connection can be reused.
+    // [Uri.scheme] is required when redirecting from non-TLS to TLS connection.
+    final transportCacheKey = '${uri.scheme}://${uri.host}:${uri.port}';
+    _ClientTransportConnectionState? transportState =
+        _transportsMap[transportCacheKey];
     if (transportState == null) {
       Future<_ClientTransportConnectionState>? initFuture =
-          _connectFutures[domain];
+          _connectFutures[transportCacheKey];
       if (initFuture == null) {
-        _connectFutures[domain] = initFuture = _connect(options);
+        _connectFutures[transportCacheKey] =
+            initFuture = _connect(options, redirects);
       }
       try {
         transportState = await initFuture;
       } catch (e) {
-        _connectFutures.remove(domain);
+        _connectFutures.remove(transportCacheKey);
         rethrow;
       }
       if (_forceClosed) {
         transportState.dispose();
       } else {
-        _transportsMap[domain] = transportState;
-        final _ = _connectFutures.remove(domain);
+        _transportsMap[transportCacheKey] = transportState;
+        final _ = _connectFutures.remove(transportCacheKey);
       }
     } else {
       // Check whether the connection is terminated, if it is, reconnecting.
       if (!transportState.transport.isOpen) {
         transportState.dispose();
-        _transportsMap[domain] = transportState = await _connect(options);
+        _transportsMap[transportCacheKey] =
+            transportState = await _connect(options, redirects);
       }
     }
     return transportState.activeTransport;
   }
 
   Future<_ClientTransportConnectionState> _connect(
     RequestOptions options,
+    List<RedirectRecord> redirects,
   ) async {
-    final uri = options.uri;
+    Uri uri = options.uri;
+    if (redirects.isNotEmpty) {
+      uri = Http2Adapter.resolveRedirectUri(uri, redirects.last.location);
+    }
     final domain = '${uri.host}:${uri.port}';
     final clientConfig = ClientSetting();
     if (onClientCreate != null) {
@@ -279,18 +292,18 @@ class _ConnectionManager implements ConnectionManager {
 class _ClientTransportConnectionState {
   _ClientTransportConnectionState(this.transport);
 
-  ClientTransportConnection transport;
+  final ClientTransportConnection transport;
+
+  bool isActive = true;
+  late DateTime latestIdleTimeStamp;
+  Timer? _timer;
 
   ClientTransportConnection get activeTransport {
     isActive = true;
     latestIdleTimeStamp = DateTime.now();
     return transport;
   }
 
-  bool isActive = true;
-  late DateTime latestIdleTimeStamp;
-  Timer? _timer;
-
   void delayClose(Duration idleTimeout, void Function() callback) {
     const duration = Duration(milliseconds: 100);
     idleTimeout = idleTimeout < duration ? duration : idleTimeout;

plugins/http2_adapter/lib/src/http2_adapter.dart

@@ -6,7 +6,6 @@ import 'dart:typed_data';
 import 'package:dio/dio.dart';
 import 'package:dio/io.dart';
 import 'package:http2/http2.dart';
-import 'package:meta/meta.dart';
 
 part 'client_setting.dart';
 
@@ -59,7 +58,7 @@ class Http2Adapter implements HttpClientAdapter {
   ) async {
     late final ClientTransportConnection transport;
     try {
-      transport = await connectionManager.getConnection(options);
+      transport = await connectionManager.getConnection(options, redirects);
     } on DioH2NotSupportedException catch (e) {
       // Fallback to use the callback
       // or to another adapter (typically IOHttpClientAdapter)
@@ -253,14 +252,8 @@ class Http2Adapter implements HttpClientAdapter {
       final url = responseHeaders.value('location');
       // An empty `location` header is considered a self redirect.
       final uri = Uri.parse(url ?? '');
-      redirects.add(
-        RedirectRecord(
-          statusCode,
-          options.method,
-          uri,
-        ),
-      );
-      final String path = resolveRedirectUri(options.uri, uri);
+      redirects.add(RedirectRecord(statusCode, options.method, uri));
+      final String path = resolveRedirectUri(options.uri, uri).toString();
       return _fetch(
         options.copyWith(
           path: path,
@@ -292,16 +285,15 @@ class Http2Adapter implements HttpClientAdapter {
         statusCodes.contains(status);
   }
 
-  @visibleForTesting
-  static String resolveRedirectUri(Uri currentUri, Uri redirectUri) {
+  static Uri resolveRedirectUri(Uri currentUri, Uri redirectUri) {
     if (redirectUri.hasScheme) {
-      /// This is a full URL which has to be redirected to as is.
-      return redirectUri.toString();
+      // This is a full URL which has to be redirected to as is.
+      return redirectUri;
     }
 
-    /// This is relative with or without leading slash and is
-    /// resolved against the URL of the original request.
-    return currentUri.resolveUri(redirectUri).toString();
+    // This is relative with or without leading slash and is resolved against
+    // the URL of the original request.
+    return currentUri.resolveUri(redirectUri);
   }
 
   @override
@@ -310,8 +302,7 @@ class Http2Adapter implements HttpClientAdapter {
   }
 }
 
-/// The exception when a connected socket for the [uri]
-/// does not support HTTP/2.
+/// The exception when a connected socket for the [uri] does not support HTTP/2.
 class DioH2NotSupportedException extends SocketException {
   const DioH2NotSupportedException(
     this.uri,

plugins/http2_adapter/test/http2_test.dart

@@ -97,6 +97,58 @@ void main() {
     expect(res.data.toString(), contains('TEST'));
   });
 
+  group(ConnectionManager, () {
+    test('returns correct connection', () async {
+      final manager = ConnectionManager();
+      final tlsConnection = await manager.getConnection(
+        RequestOptions(path: 'https://flutter.cn'),
+        [],
+      );
+      final tlsWithSameHostRedirects = await manager.getConnection(
+        RequestOptions(path: 'https://flutter.cn'),
+        [
+          RedirectRecord(301, 'GET', Uri.parse('https://flutter.cn/404')),
+        ],
+      );
+      final tlsDifferentHostRedirects = await manager.getConnection(
+        RequestOptions(path: 'https://flutter.cn'),
+        [
+          RedirectRecord(301, 'GET', Uri.parse('https://flutter.dev')),
+        ],
+      );
+      final tlsDifferentHostsRedirects = await manager.getConnection(
+        RequestOptions(path: 'https://flutter.cn'),
+        [
+          RedirectRecord(301, 'GET', Uri.parse('https://flutter.dev')),
+          RedirectRecord(301, 'GET', Uri.parse('https://flutter.dev/404')),
+        ],
+      );
+      final nonTLSConnection = await manager.getConnection(
+        RequestOptions(path: 'http://flutter.cn'),
+        [],
+      );
+      final nonTLSConnectionWithTLSRedirects = await manager.getConnection(
+        RequestOptions(path: 'http://flutter.cn'),
+        [
+          RedirectRecord(301, 'GET', Uri.parse('https://flutter.cn/')),
+        ],
+      );
+      final differentHostConnection = await manager.getConnection(
+        RequestOptions(path: 'https://flutter.dev'),
+        [],
+      );
+      expect(tlsConnection == tlsWithSameHostRedirects, true);
+      expect(tlsConnection == tlsDifferentHostRedirects, false);
+      expect(tlsConnection == tlsDifferentHostsRedirects, false);
+      expect(tlsConnection == nonTLSConnection, false);
+      expect(tlsConnection == nonTLSConnectionWithTLSRedirects, true);
+      expect(tlsConnection == differentHostConnection, false);
+      expect(tlsDifferentHostRedirects == differentHostConnection, true);
+      expect(tlsDifferentHostsRedirects == differentHostConnection, true);
+      expect(nonTLSConnection == nonTLSConnectionWithTLSRedirects, false);
+    });
+  });
+
   group(ProxyConnectedPredicate, () {
     group('defaultProxyConnectedPredicate', () {
       test(

plugins/http2_adapter/test/redirect_test.dart

@@ -2,15 +2,14 @@ import 'package:dio_http2_adapter/dio_http2_adapter.dart';
 import 'package:test/test.dart';
 
 void main() {
-  group(Http2Adapter.resolveRedirectUri, () {
+  group('Http2Adapter.resolveRedirectUri', () {
     test('empty location', () async {
       final current = Uri.parse('https://example.com');
       final result = Http2Adapter.resolveRedirectUri(
         current,
         Uri.parse(''),
       );
-
-      expect(result, current.toString());
+      expect(result.toString(), current.toString());
     });
 
     test('relative location 1', () async {
@@ -19,16 +18,15 @@ void main() {
         Uri.parse('/bar'),
       );
 
-      expect(result, 'https://example.com/bar');
+      expect(result.toString(), 'https://example.com/bar');
     });
 
     test('relative location 2', () async {
       final result = Http2Adapter.resolveRedirectUri(
         Uri.parse('https://example.com/foo'),
         Uri.parse('../bar'),
       );
-
-      expect(result, 'https://example.com/bar');
+      expect(result.toString(), 'https://example.com/bar');
     });
 
     test('different location', () async {
@@ -38,8 +36,7 @@ void main() {
         current,
         Uri.parse(target),
       );
-
-      expect(result, target);
+      expect(result.toString(), target);
     });
   });
 }