Refactor temporary file handling for efficiency (#1865)

Author: jcollins-g

bin/dartdoc.dart

@@ -11,6 +11,7 @@ import 'package:args/args.dart';
 import 'package:dartdoc/dartdoc.dart';
 import 'package:dartdoc/src/html/html_generator.dart';
 import 'package:dartdoc/src/logging.dart';
+import 'package:dartdoc/src/tool_runner.dart';
 import 'package:stack_trace/stack_trace.dart';
 
 class DartdocProgramOptionContext extends DartdocGeneratorOptionContext
@@ -94,8 +95,9 @@ void main(List<String> arguments) async {
       }
     }, when: config.asyncStackTraces);
   } finally {
-    // Clear out any cached tool snapshots.
+    // Clear out any cached tool snapshots and temporary directories.
     SnapshotCache.instance.dispose();
+    ToolTempFileTracker.instance.dispose();
   }
 }
 

lib/src/dartdoc_options.dart

@@ -21,6 +21,7 @@ import 'package:analyzer/dart/element/element.dart';
 import 'package:args/args.dart';
 import 'package:dartdoc/dartdoc.dart';
 import 'package:dartdoc/src/io_utils.dart';
+import 'package:dartdoc/src/tool_runner.dart';
 import 'package:dartdoc/src/tuple.dart';
 import 'package:path/path.dart' as pathLib;
 import 'package:yaml/yaml.dart';
@@ -243,11 +244,12 @@ class SnapshotCache {
     return snapshots[toolPath];
   }
 
-  void dispose() {
+  Future<void> dispose() {
+    _instance = null;
     if (snapshotCache != null && snapshotCache.existsSync()) {
-      snapshotCache.deleteSync(recursive: true);
+      return snapshotCache.delete(recursive: true);
     }
-    _instance = null;
+    return null;
   }
 }
 
@@ -288,12 +290,16 @@ class DartToolDefinition extends ToolDefinition {
 class ToolConfiguration {
   final Map<String, ToolDefinition> tools;
 
+  ToolRunner _runner;
+  ToolRunner get runner => _runner ??= ToolRunner(this);
+
   ToolConfiguration._(this.tools);
 
   static ToolConfiguration get empty {
     return new ToolConfiguration._({});
   }
 
+  // TODO(jcollins-g): consider caching these.
   static ToolConfiguration fromYamlMap(
       YamlMap yamlMap, pathLib.Context pathContext) {
     var newToolDefinitions = <String, ToolDefinition>{};

lib/src/markdown_processor.dart

@@ -15,7 +15,6 @@ import 'package:dartdoc/src/tuple.dart';
 import 'package:dartdoc/src/warnings.dart';
 import 'package:html/parser.dart' show parse;
 import 'package:markdown/markdown.dart' as md;
-import 'package:quiver/iterables.dart' as quiverIterables;
 
 const validHtmlTags = const [
   "a",

lib/src/model.dart

@@ -57,7 +57,6 @@ import 'package:dartdoc/src/markdown_processor.dart' show Documentation;
 import 'package:dartdoc/src/model_utils.dart';
 import 'package:dartdoc/src/package_meta.dart' show PackageMeta, FileContents;
 import 'package:dartdoc/src/special_elements.dart';
-import 'package:dartdoc/src/tool_runner.dart';
 import 'package:dartdoc/src/tuple.dart';
 import 'package:dartdoc/src/utils.dart';
 import 'package:dartdoc/src/warnings.dart';
@@ -4121,9 +4120,6 @@ abstract class ModelElement extends Canonicalization
   /// 2018-09-18T21:15+00:00
   Future<String> _evaluateTools(String rawDocs) async {
     if (config.allowTools) {
-      var runner = new ToolRunner(config.tools, (String message) async {
-        warn(PackageWarning.toolError, message: message);
-      });
       int invocationIndex = 0;
       return await _replaceAllMappedAsync(rawDocs, basicToolRegExp,
           (basicMatch) async {
@@ -4138,7 +4134,8 @@ abstract class ModelElement extends Canonicalization
         // Count the number of invocations of tools in this dartdoc block,
         // so that tools can differentiate different blocks from each other.
         invocationIndex++;
-        return await runner.run(args,
+        return await config.tools.runner.run(args,
+            (String message) async => warn(PackageWarning.toolError, message: message),
             content: basicMatch[2],
             environment: {
               'SOURCE_LINE': lineAndColumn?.item1?.toString(),
@@ -4153,7 +4150,7 @@ abstract class ModelElement extends Canonicalization
               'ELEMENT_NAME': fullyQualifiedNameWithoutLibrary,
               'INVOCATION_INDEX': invocationIndex.toString(),
             }..removeWhere((key, value) => value == null));
-      }).whenComplete(runner.dispose);
+      });
     } else {
       return rawDocs;
     }
@@ -4783,7 +4780,7 @@ class PackageGraph {
     // specialClasses handler so when we find them, they get added.
     specialClasses = new SpecialClasses();
     // Go through docs of every ModelElement in package to pre-build the macros
-    // index.
+    // index.  Uses toList() in order to get all the precaching on the stack.
     List<Future> precacheFutures = precacheLocalDocs().toList();
     for (Future f in precacheFutures) await f;
     _localDocumentationBuilt = true;

lib/src/tool_runner.dart

@@ -19,60 +19,49 @@ typedef FakeResultCallback = String Function(String tool,
 /// limiting both parallelization and the number of open temporary files.
 final MultiFutureTracker _toolTracker = new MultiFutureTracker(4);
 
-/// A helper class for running external tools.
-class ToolRunner {
-  /// Creates a new ToolRunner.
-  ///
-  /// Takes a [toolConfiguration] that describes all of the available tools.
-  /// An optional `errorCallback` will be called for each error message
-  /// generated by the tool.
-  ToolRunner(this.toolConfiguration, [this._errorCallback]);
+/// Can be called when the ToolRunner is no longer needed.
+///
+/// This will remove any temporary files created by the tool runner.
+class ToolTempFileTracker {
+  final Directory temporaryDirectory;
 
-  final ToolConfiguration toolConfiguration;
-  final ToolErrorCallback _errorCallback;
-  int _temporaryFileCount = 0;
+  ToolTempFileTracker._() : temporaryDirectory = Directory.systemTemp.createTempSync('dartdoc_tools_');
 
-  Future<Directory> _temporaryDirectory;
-  Future<Directory> get temporaryDirectory {
-    if (_temporaryDirectory == null) {
-      _temporaryDirectory = Directory.systemTemp.createTemp('dartdoc_tools_');
-    }
-    return _temporaryDirectory;
-  }
+  static ToolTempFileTracker _instance;
+  static ToolTempFileTracker get instance => _instance ??= ToolTempFileTracker._();
 
-  void _error(String message) {
-    if (_errorCallback != null) {
-      _errorCallback(message);
-    }
-  }
-
-  Future<File> _createTemporaryFile() async {
+  int _temporaryFileCount = 0;
+  Future<File> createTemporaryFile() async {
     _temporaryFileCount++;
     File tempFile = new File(pathLib.join(
-        (await temporaryDirectory).absolute.path,
+        temporaryDirectory.absolute.path,
         'input_$_temporaryFileCount'));
     await tempFile.create(recursive: true);
     return tempFile;
   }
 
-  /// Must be called when the ToolRunner is no longer needed. Ideally, this is
-  /// called in the finally section of a try/finally.
-  ///
-  /// This will remove any temporary files created by the tool runner.
-  void dispose() {
-    if (_temporaryDirectory != null) disposeAsync(_temporaryDirectory);
-  }
-
-  /// Avoid blocking on I/O for cleanups.
-  static Future<void> disposeAsync(Future<Directory> temporaryDirectory) async {
-    Directory tempDir = await temporaryDirectory;
-    if (await tempDir.exists()) {
-      return tempDir.delete(recursive: true);
+  /// Call once no more files are to be created.
+  Future<void> dispose() async {
+    if (temporaryDirectory.existsSync()) {
+      return temporaryDirectory.delete(recursive: true);
     }
   }
+}
+
+/// A helper class for running external tools.
+class ToolRunner {
+  /// Creates a new ToolRunner.
+  ///
+  /// Takes a [toolConfiguration] that describes all of the available tools.
+  /// An optional `errorCallback` will be called for each error message
+  /// generated by the tool.
+  ToolRunner(this.toolConfiguration);
+
+  final ToolConfiguration toolConfiguration;
 
   void _runSetup(
-      String name, ToolDefinition tool, Map<String, String> environment) async {
+      String name, ToolDefinition tool, Map<String, String> environment,
+      ToolErrorCallback toolErrorCallback) async {
     bool isDartSetup = ToolDefinition.isDartExecutable(tool.setupCommand[0]);
     var args = tool.setupCommand.toList();
     String commandPath;
@@ -82,18 +71,19 @@ class ToolRunner {
     } else {
       commandPath = args.removeAt(0);
     }
-    await _runProcess(name, '', commandPath, args, environment);
+    await _runProcess(name, '', commandPath, args, environment, toolErrorCallback);
     tool.setupComplete = true;
   }
 
   Future<String> _runProcess(String name, String content, String commandPath,
-      List<String> args, Map<String, String> environment) async {
+      List<String> args, Map<String, String> environment,
+      ToolErrorCallback toolErrorCallback) async {
     String commandString() => ([commandPath] + args).join(' ');
     try {
       ProcessResult result =
           await Process.run(commandPath, args, environment: environment);
       if (result.exitCode != 0) {
-        _error('Tool "$name" returned non-zero exit code '
+        toolErrorCallback('Tool "$name" returned non-zero exit code '
             '(${result.exitCode}) when run as '
             '"${commandString()}" from ${Directory.current}\n'
             'Input to $name was:\n'
@@ -104,7 +94,7 @@ class ToolRunner {
         return result.stdout;
       }
     } on ProcessException catch (exception) {
-      _error('Failed to run tool "$name" as '
+      toolErrorCallback('Failed to run tool "$name" as '
           '"${commandString()}": $exception\n'
           'Input to $name was:\n'
           '$content');
@@ -118,26 +108,26 @@ class ToolRunner {
   ///
   /// The [args] must not be null, and it must have at least one member (the name
   /// of the tool).
-  Future<String> run(List<String> args,
+  Future<String> run(List<String> args, ToolErrorCallback toolErrorCallback,
       {String content, Map<String, String> environment}) async {
     Future runner;
     // Prevent too many tools from running simultaneously.
     await _toolTracker.addFutureFromClosure(() {
-      runner = _run(args, content: content, environment: environment);
+      runner = _run(args, toolErrorCallback, content: content, environment: environment);
       return runner;
     });
     return runner;
   }
 
-  Future<String> _run(List<String> args,
+  Future<String> _run(List<String> args, ToolErrorCallback toolErrorCallback,
       {String content, Map<String, String> environment}) async {
     assert(args != null);
     assert(args.isNotEmpty);
     content ??= '';
     environment ??= <String, String>{};
     var tool = args.removeAt(0);
     if (!toolConfiguration.tools.containsKey(tool)) {
-      _error('Unable to find definition for tool "$tool" in tool map. '
+      toolErrorCallback('Unable to find definition for tool "$tool" in tool map. '
           'Did you add it to dartdoc_options.yaml?');
       return '';
     }
@@ -152,7 +142,7 @@ class ToolRunner {
     // file before running the tool synchronously.
 
     // Write the content to a temp file.
-    var tmpFile = await _createTemporaryFile();
+    var tmpFile = await ToolTempFileTracker.instance.createTemporaryFile();
     await tmpFile.writeAsString(content);
 
     // Substitute the temp filename for the "$INPUT" token, and all of the other
@@ -188,7 +178,7 @@ class ToolRunner {
     }
 
     if (toolDefinition.setupCommand != null && !toolDefinition.setupComplete)
-      await _runSetup(tool, toolDefinition, envWithInput);
+      await _runSetup(tool, toolDefinition, envWithInput, toolErrorCallback);
 
     argsWithInput = toolArgs + argsWithInput;
     var commandPath;
@@ -201,13 +191,14 @@ class ToolRunner {
     } else {
       commandPath = argsWithInput.removeAt(0);
     }
+
     if (callCompleter != null) {
       return _runProcess(
-              tool, content, commandPath, argsWithInput, envWithInput)
+              tool, content, commandPath, argsWithInput, envWithInput, toolErrorCallback)
           .whenComplete(callCompleter);
     } else {
       return _runProcess(
-          tool, content, commandPath, argsWithInput, envWithInput);
+          tool, content, commandPath, argsWithInput, envWithInput, toolErrorCallback);
     }
   }
 }

test/tool_runner_test.dart

@@ -20,6 +20,8 @@ void main() {
   File setupFile;
 
   ToolRunner runner;
+  ToolTempFileTracker tracker;
+  ToolErrorCallback errorCallback;
   final errors = <String>[];
 
   setUpAll(() async {
@@ -81,11 +83,12 @@ echo:
       'missing': new ToolDefinition(['/a/missing/executable'], null, "missing"),
     });
 
-    runner = new ToolRunner(toolMap, (String message) => errors.add(message));
+    runner = new ToolRunner(toolMap);
+    errorCallback = (String message) => errors.add(message);
   });
   tearDownAll(() {
     tempDir?.deleteSync(recursive: true);
-    runner?.dispose();
+    tracker?.dispose();
     SnapshotCache.instance.dispose();
     setupFile = null;
     tempDir = null;
@@ -100,6 +103,7 @@ echo:
     test('can invoke a Dart tool, and second run is a snapshot.', () async {
       var result = await runner.run(
         ['drill', r'--file=$INPUT'],
+        errorCallback,
         content: 'TEST INPUT',
       );
       expect(errors, isEmpty);
@@ -109,6 +113,7 @@ echo:
       expect(setupFile.existsSync(), isFalse);
       result = await runner.run(
         ['drill', r'--file=$INPUT'],
+        errorCallback,
         content: 'TEST INPUT 2',
       );
       expect(errors, isEmpty);
@@ -120,6 +125,7 @@ echo:
     test('can invoke a Dart tool', () async {
       var result = await runner.run(
         ['drill', r'--file=$INPUT'],
+        errorCallback,
         content: 'TEST INPUT',
       );
       expect(errors, isEmpty);
@@ -131,6 +137,7 @@ echo:
     test('can invoke a non-Dart tool', () async {
       String result = await runner.run(
         ['non_dart', '--version'],
+        errorCallback,
         content: 'TEST INPUT',
       );
       expect(errors, isEmpty);
@@ -139,6 +146,7 @@ echo:
     test('can invoke a pre-snapshotted tool', () async {
       var result = await runner.run(
         ['snapshot_drill', r'--file=$INPUT'],
+        errorCallback,
         content: 'TEST INPUT',
       );
       expect(errors, isEmpty);
@@ -148,6 +156,7 @@ echo:
     test('can invoke a tool with a setup action', () async {
       var result = await runner.run(
         ['setup_drill', r'--file=$INPUT'],
+        errorCallback,
         content: 'TEST INPUT',
       );
       expect(errors, isEmpty);
@@ -158,6 +167,7 @@ echo:
     test('fails if tool not in tool map', () async {
       String result = await runner.run(
         ['hammer', r'--file=$INPUT'],
+        errorCallback,
         content: 'TEST INPUT',
       );
       expect(errors, isNotEmpty);
@@ -168,6 +178,7 @@ echo:
     test('fails if tool returns non-zero status', () async {
       String result = await runner.run(
         ['drill', r'--file=/a/missing/file'],
+        errorCallback,
         content: 'TEST INPUT',
       );
       expect(errors, isNotEmpty);
@@ -177,6 +188,7 @@ echo:
     test("fails if tool in tool map doesn't exist", () async {
       String result = await runner.run(
         ['missing'],
+        errorCallback,
         content: 'TEST INPUT',
       );
       expect(errors, isNotEmpty);