[flutter_tools] cache result of BotDetector in persistent tool state (#52325)

The Azure bot detection can take up to a second to determine if a client is/isn't a bot. To prevent this from slowing down all flutter commands, we can cache the results in the persistent tool state - since we don't expect the same client id to ever become a bot or stop being a bot

packages/flutter_tools/lib/runner.dart

@@ -35,8 +35,6 @@ Future<int> run(
   String flutterVersion,
   Map<Type, Generator> overrides,
 }) async {
-  reportCrashes ??= !await globals.isRunningOnBot;
-
   if (muteCommandLogging) {
     // Remove the verbose option; for help and doctor, users don't need to see
     // verbose logs.
@@ -48,6 +46,8 @@ Future<int> run(
   commands.forEach(runner.addCommand);
 
   return runInContext<int>(() async {
+    reportCrashes ??= !await globals.isRunningOnBot;
+
     // Initialize the system locale.
     final String systemLocale = await intl_standalone.findSystemLocale();
     intl.Intl.defaultLocale = intl.Intl.verifiedLocale(

packages/flutter_tools/lib/src/base/bot_detector.dart

@@ -5,6 +5,7 @@
 import 'package:meta/meta.dart';
 import 'package:platform/platform.dart';
 
+import '../persistent_tool_state.dart';
 import 'io.dart';
 import 'net.dart';
 
@@ -12,20 +13,21 @@ class BotDetector {
   BotDetector({
     @required HttpClientFactory httpClientFactory,
     @required Platform platform,
+    @required PersistentToolState persistentToolState,
   }) :
     _platform = platform,
     _azureDetector = AzureDetector(
       httpClientFactory: httpClientFactory,
-    );
+    ),
+    _persistentToolState = persistentToolState;
 
   final Platform _platform;
   final AzureDetector _azureDetector;
-
+  final PersistentToolState _persistentToolState;
-  bool _isRunningOnBot;
 
   Future<bool> get isRunningOnBot async {
-    if (_isRunningOnBot != null) {
+    if (_persistentToolState.isRunningOnBot != null) {
-      return _isRunningOnBot;
+      return _persistentToolState.isRunningOnBot;
     }
     if (
       // Explicitly stated to not be a bot.
@@ -36,10 +38,10 @@ class BotDetector {
       // When set, GA logs to a local file (normally for tests) so we don't need to filter.
       || _platform.environment.containsKey('FLUTTER_ANALYTICS_LOG_FILE')
     ) {
-      return _isRunningOnBot = false;
+      return _persistentToolState.isRunningOnBot = false;
     }
 
-    return _isRunningOnBot = _platform.environment['BOT'] == 'true'
+    return _persistentToolState.isRunningOnBot = _platform.environment['BOT'] == 'true'
 
       // https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables
       || _platform.environment['TRAVIS'] == 'true'
@@ -94,7 +96,7 @@ class AzureDetector {
       return _isRunningOnAzure;
     }
     final HttpClient client = _httpClientFactory()
-      ..connectionTimeout = const Duration(seconds: 1);
+      ..connectionTimeout = const Duration(milliseconds: 250);
     try {
       final HttpClientRequest request = await client.getUrl(
         Uri.parse(_serviceUrl),

packages/flutter_tools/lib/src/globals.dart

@@ -80,6 +80,7 @@ XCDevice get xcdevice => context.get<XCDevice>();
 final BotDetector _defaultBotDetector = BotDetector(
   httpClientFactory: context.get<HttpClientFactory>() ?? () => HttpClient(),
   platform: platform,
+  persistentToolState: persistentToolState,
 );
 
 BotDetector get botDetector => context.get<BotDetector>() ?? _defaultBotDetector;

packages/flutter_tools/lib/src/persistent_tool_state.dart

@@ -46,6 +46,9 @@ abstract class PersistentToolState {
 
   /// Update the last active version for a given [channel].
   void updateLastActiveVersion(String fullGitHash, Channel channel);
+
+  /// Whether this client was already determined to be or not be a bot.
+  bool isRunningOnBot;
 }
 
 class _DefaultPersistentToolState implements PersistentToolState {
@@ -78,6 +81,7 @@ class _DefaultPersistentToolState implements PersistentToolState {
     Channel.beta: 'last-active-beta-version',
     Channel.stable: 'last-active-stable-version'
   };
+  static const String _kBotKey = 'is-bot';
 
   final Config _config;
 
@@ -108,4 +112,10 @@ class _DefaultPersistentToolState implements PersistentToolState {
   String _versionKeyFor(Channel channel) {
     return _lastActiveVersionKeys[channel];
   }
+
+  @override
+  bool get isRunningOnBot => _config.getValue(_kBotKey) as bool;
+
+  @override
+  set isRunningOnBot(bool value) => _config.setValue(_kBotKey, value);
 }

packages/flutter_tools/test/general.shard/base/bot_detector_test.dart

@@ -2,8 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'package:file/memory.dart';
 import 'package:flutter_tools/src/base/bot_detector.dart';
 import 'package:flutter_tools/src/base/io.dart';
+import 'package:flutter_tools/src/base/logger.dart';
+import 'package:flutter_tools/src/persistent_tool_state.dart';
 import 'package:mockito/mockito.dart';
 import 'package:platform/platform.dart';
 
@@ -18,6 +21,7 @@ void main() {
     MockHttpClientRequest mockHttpClientRequest;
     MockHttpHeaders mockHttpHeaders;
     BotDetector botDetector;
+    PersistentToolState persistentToolState;
 
     setUp(() {
       fakePlatform = FakePlatform()..environment = <String, String>{};
@@ -25,9 +29,14 @@ void main() {
       mockHttpClient = MockHttpClient();
       mockHttpClientRequest = MockHttpClientRequest();
       mockHttpHeaders = MockHttpHeaders();
+      persistentToolState = PersistentToolState.test(
+        directory: MemoryFileSystem.test().currentDirectory,
+        logger: BufferLogger.test(),
+      );
       botDetector = BotDetector(
         platform: fakePlatform,
         httpClientFactory: () => mockHttpClient,
+        persistentToolState: persistentToolState,
       );
     });
 
@@ -35,13 +44,17 @@ void main() {
       testWithoutContext('returns false unconditionally if BOT=false is set', () async {
         fakePlatform.environment['BOT'] = 'false';
         fakePlatform.environment['TRAVIS'] = 'true';
+
         expect(await botDetector.isRunningOnBot, isFalse);
+        expect(persistentToolState.isRunningOnBot, isFalse);
       });
 
       testWithoutContext('returns false unconditionally if FLUTTER_HOST is set', () async {
         fakePlatform.environment['FLUTTER_HOST'] = 'foo';
         fakePlatform.environment['TRAVIS'] = 'true';
+
         expect(await botDetector.isRunningOnBot, isFalse);
+        expect(persistentToolState.isRunningOnBot, isFalse);
       });
 
       testWithoutContext('returns false with and without a terminal attached', () async {
@@ -52,12 +65,14 @@ void main() {
         expect(await botDetector.isRunningOnBot, isFalse);
         mockStdio.stdout.hasTerminal = false;
         expect(await botDetector.isRunningOnBot, isFalse);
+        expect(persistentToolState.isRunningOnBot, isFalse);
       });
 
       testWithoutContext('can test analytics outputs on bots when outputting to a file', () async {
         fakePlatform.environment['TRAVIS'] = 'true';
         fakePlatform.environment['FLUTTER_ANALYTICS_LOG_FILE'] = '/some/file';
         expect(await botDetector.isRunningOnBot, isFalse);
+        expect(persistentToolState.isRunningOnBot, isFalse);
       });
 
       testWithoutContext('returns true when azure metadata is reachable', () async {
@@ -65,12 +80,31 @@ void main() {
           return Future<HttpClientRequest>.value(mockHttpClientRequest);
         });
         when(mockHttpClientRequest.headers).thenReturn(mockHttpHeaders);
+
         expect(await botDetector.isRunningOnBot, isTrue);
+        expect(persistentToolState.isRunningOnBot, isTrue);
+      });
+
+      testWithoutContext('caches azure bot detection results across instances', () async {
+        when(mockHttpClient.getUrl(any)).thenAnswer((_) {
+          return Future<HttpClientRequest>.value(mockHttpClientRequest);
+        });
+        when(mockHttpClientRequest.headers).thenReturn(mockHttpHeaders);
+
+        expect(await botDetector.isRunningOnBot, isTrue);
+        expect(await BotDetector(
+          platform: fakePlatform,
+          httpClientFactory: () => mockHttpClient,
+          persistentToolState: persistentToolState,
+        ).isRunningOnBot, isTrue);
+        verify(mockHttpClient.getUrl(any)).called(1);
       });
 
       testWithoutContext('returns true when running on borg', () async {
         fakePlatform.environment['BORG_ALLOC_DIR'] = 'true';
+
         expect(await botDetector.isRunningOnBot, isTrue);
+        expect(persistentToolState.isRunningOnBot, isTrue);
       });
     });
   });
@@ -94,6 +128,7 @@ void main() {
       when(mockHttpClient.getUrl(any)).thenAnswer((_) {
         throw const SocketException('HTTP connection timed out');
       });
+
       expect(await azureDetector.isRunningOnAzure, isFalse);
     });
 
@@ -102,6 +137,7 @@ void main() {
         return Future<HttpClientRequest>.value(mockHttpClientRequest);
       });
       when(mockHttpClientRequest.headers).thenReturn(mockHttpHeaders);
+
       expect(await azureDetector.isRunningOnAzure, isTrue);
     });
   });