Skip to content

Commit

Permalink
Add global analysis driver pool to protect usages (#3313)
Browse files Browse the repository at this point in the history 
Fixes #3312

If the driver is used in between calls to `changeFile` and before `applyPendingChanges` completes, then you can get `InconsistentAnalysisError`s. This change ensures we won't do that through a new single resource pool shared by all users of the driver.

- Guard all accesses to the driver and session with the new resource.
  - During the resolve, this only grabs the resource once it is fully ready for all of the `changeFile` calls and subsequent call to `applyPendingFileChanges`. This prevents deadlocks compared to a global resolution pool, which may call out to `package:build` apis which can deadlock.
  - This shouldn't cause significant slowdown, most of the code using the pool has a fixed number of async actions it waits on, and none of those actions might need to wait on a build step to complete (they are all async calls to analyzer apis, not build apis).
- Keeps in place the per action resolve pool, still needed for managing updates to `_entryPoints`.
  • Loading branch information
@jakemac53
jakemac53 committed May 26, 2022
1 parent 7067c3b commit 7a384f3
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 67 deletions.
5 changes: 4 additions & 1 deletion build_resolvers/CHANGELOG.md
View file
@@ -1,4 +1,7 @@
## 2.0.9-dev
## 2.0.9

- Fix a new case of `InconsistentAnalysisException` errors that can occur with
the newer analyzer.

## 2.0.8

Expand Down
26 changes: 16 additions & 10 deletions build_resolvers/lib/src/build_asset_uri_resolver.dart
View file
Expand Up @@ -2,6 +2,7 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:async';
import 'dart:collection';

import 'package:analyzer/dart/analysis/utilities.dart';
Expand Down Expand Up @@ -49,13 +50,17 @@ class BuildAssetUriResolver extends UriResolver {
/// input, subsequent calls to a resolver, or a transitive import thereof.
final _buildStepTransitivelyResolvedAssets = <BuildStep, HashSet<AssetId>>{};

/// Updates [resourceProvider] and [driver] with updated versions of
/// [entryPoints].
/// Updates [resourceProvider] and the analysis driver given by
/// `withDriverResource` with updated versions of [entryPoints].
///
/// If [transitive], then all the transitive imports from [entryPoints] are
/// also updated.
Future<void> performResolve(BuildStep buildStep, List<AssetId> entryPoints,
AnalysisDriverForPackageBuild driver,
Future<void> performResolve(
BuildStep buildStep,
List<AssetId> entryPoints,
Future<void> Function(
FutureOr<void> Function(AnalysisDriverForPackageBuild))
withDriverResource,
{required bool transitive}) async {
final transitivelyResolved = _buildStepTransitivelyResolvedAssets
.putIfAbsent(buildStep, () => HashSet());
Expand All @@ -74,13 +79,14 @@ class BuildAssetUriResolver extends UriResolver {
for (final id in uncrawledIds)
(await _updateCachedAssetState(id, buildStep))!
];

for (final state in assetStates) {
if (_needsChangeFile.remove(state.path)) {
driver.changeFile(state.path);
await withDriverResource((driver) async {
for (final state in assetStates) {
if (_needsChangeFile.remove(state.path)) {
driver.changeFile(state.path);
}
}
}
await driver.applyPendingFileChanges();
await driver.applyPendingFileChanges();
});
}

/// Updates the internal state for [id], if it has changed.
Expand Down
134 changes: 79 additions & 55 deletions build_resolvers/lib/src/resolver.dart
View file
Expand Up @@ -2,6 +2,7 @@
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

import 'dart:async';
import 'dart:collection';
import 'dart:convert';
import 'dart:io';
Expand Down Expand Up @@ -40,11 +41,12 @@ Future<String> _packagePath(String package) async {
/// down from entrypoints.
class PerActionResolver implements ReleasableResolver {
final AnalyzerResolver _delegate;
final Pool _driverPool;
final BuildStep _step;

final _entryPoints = <AssetId>{};

PerActionResolver(this._delegate, this._step);
PerActionResolver(this._delegate, this._driverPool, this._step);

@override
Stream<LibraryElement> get libraries async* {
Expand Down Expand Up @@ -124,22 +126,25 @@ class PerActionResolver implements ReleasableResolver {
allowSyntaxErrors: allowSyntaxErrors);
});

// Ensures that we finish resolving one thing before attempting to resolve
// another, otherwise there are race conditions with `_entryPoints` being
// updated before it is actually ready, or resolved more than once.
final _resolvePool = Pool(1);
// Ensures we only resolve one entrypoint at a time from the same build step,
// otherwise there are race conditions with `_entryPoints` being updated
// before it is actually ready, or resolving entrypoints more than once.
final Pool _perActionResolvePool = Pool(1);
Future<void> _resolveIfNecessary(AssetId id, {required bool transitive}) =>
_resolvePool.withResource(() async {
_perActionResolvePool.withResource(() async {
if (!_entryPoints.contains(id)) {
// We only want transitively resolved ids in `_entrypoints`.
if (transitive) _entryPoints.add(id);

// the resolver will only visit assets that haven't been resolved in this
// step yet
// the resolver will only visit assets that haven't been resolved in
// this step yet
await _step.trackStage(
'Resolving library $id',
() => _delegate._uriResolver.performResolve(
_step, [id], _delegate._driver,
_step,
[id],
(withDriver) => _driverPool
.withResource(() => withDriver(_delegate._driver)),
transitive: transitive));
}
});
Expand All @@ -158,21 +163,23 @@ class PerActionResolver implements ReleasableResolver {
class AnalyzerResolver implements ReleasableResolver {
final BuildAssetUriResolver _uriResolver;
final AnalysisDriverForPackageBuild _driver;
final Pool _driverPool;

AnalyzerResolver(this._driver, this._uriResolver);
AnalyzerResolver(this._driver, this._driverPool, this._uriResolver);

@override
Future<bool> isLibrary(AssetId assetId) async {
if (assetId.extension != '.dart') return false;
if (!_driver.isUriOfExistingFile(assetId.uri)) return false;
var result =
_driver.currentSession.getFile(assetPath(assetId)) as FileResult;
return !result.isPart;
return _driverPool.withResource(() {
if (!_driver.isUriOfExistingFile(assetId.uri)) return false;
var result =
_driver.currentSession.getFile(assetPath(assetId)) as FileResult;
return !result.isPart;
});
}

@override
Future<AstNode?> astNodeFor(Element element, {bool resolve = false}) async {
var session = _driver.currentSession;
final library = element.library;
if (library == null) {
// Invalid elements (e.g. an MultiplyDefinedElement) are not part of any
Expand All @@ -181,49 +188,58 @@ class AnalyzerResolver implements ReleasableResolver {
}
var path = library.source.fullName;

if (resolve) {
return (await session.getResolvedLibrary(path) as ResolvedLibraryResult)
.getElementDeclaration(element)
?.node;
} else {
return (session.getParsedLibrary(path) as ParsedLibraryResult)
.getElementDeclaration(element)
?.node;
}
return _driverPool.withResource(() async {
var session = _driver.currentSession;
if (resolve) {
return (await session.getResolvedLibrary(path) as ResolvedLibraryResult)
.getElementDeclaration(element)
?.node;
} else {
return (session.getParsedLibrary(path) as ParsedLibraryResult)
.getElementDeclaration(element)
?.node;
}
});
}

@override
Future<CompilationUnit> compilationUnitFor(AssetId assetId,
{bool allowSyntaxErrors = false}) async {
if (!_driver.isUriOfExistingFile(assetId.uri)) {
throw AssetNotFoundException(assetId);
}
{bool allowSyntaxErrors = false}) {
return _driverPool.withResource(() async {
if (!_driver.isUriOfExistingFile(assetId.uri)) {
throw AssetNotFoundException(assetId);
}

var path = assetPath(assetId);
var parsedResult =
_driver.currentSession.getParsedUnit(path) as ParsedUnitResult;
if (!allowSyntaxErrors && parsedResult.errors.isNotEmpty) {
throw SyntaxErrorInAssetException(assetId, [parsedResult]);
}
return parsedResult.unit;
var path = assetPath(assetId);

var parsedResult =
_driver.currentSession.getParsedUnit(path) as ParsedUnitResult;
if (!allowSyntaxErrors && parsedResult.errors.isNotEmpty) {
throw SyntaxErrorInAssetException(assetId, [parsedResult]);
}
return parsedResult.unit;
});
}

@override
Future<LibraryElement> libraryFor(AssetId assetId,
{bool allowSyntaxErrors = false}) async {
var uri = assetId.uri;
if (!_driver.isUriOfExistingFile(uri)) {
throw AssetNotFoundException(assetId);
}
final library = await _driverPool.withResource(() async {
var uri = assetId.uri;
if (!_driver.isUriOfExistingFile(uri)) {
throw AssetNotFoundException(assetId);
}

var path = assetPath(assetId);
var parsedResult = _driver.currentSession.getParsedUnit(path);
if (parsedResult is! ParsedUnitResult || parsedResult.isPart) {
throw NonLibraryAssetException(assetId);
}
var path = assetPath(assetId);
var parsedResult = _driver.currentSession.getParsedUnit(path);
if (parsedResult is! ParsedUnitResult || parsedResult.isPart) {
throw NonLibraryAssetException(assetId);
}

return await _driver.currentSession.getLibraryByUri(uri.toString())
as LibraryElementResult;
});

final library = await _driver.currentSession.getLibraryByUri(uri.toString())
as LibraryElementResult;
if (!allowSyntaxErrors) {
final errors = await _syntacticErrorsFor(library.element);
if (errors.isNotEmpty) {
Expand Down Expand Up @@ -254,14 +270,16 @@ class AnalyzerResolver implements ReleasableResolver {

final relevantResults = <ErrorsResult>[];

for (final path in paths) {
final result = await _driver.currentSession.getErrors(path);
if (result is ErrorsResult &&
result.errors.any(
(error) => error.errorCode.type == ErrorType.SYNTACTIC_ERROR)) {
relevantResults.add(result);
await _driverPool.withResource(() async {
for (final path in paths) {
final result = await _driver.currentSession.getErrors(path);
if (result is ErrorsResult &&
result.errors.any(
(error) => error.errorCode.type == ErrorType.SYNTACTIC_ERROR)) {
relevantResults.add(result);
}
}
}
});

return relevantResults;
}
Expand Down Expand Up @@ -304,6 +322,12 @@ class AnalyzerResolvers implements Resolvers {
/// Nullable, the default analysis options are used if not provided.
final AnalysisOptions _analysisOptions;

/// Used to protect all usages of the analysis driver from
/// `InconsistentAnalysisException` errors, which might occur if we are in the
/// middle of some calls to `changeFile` but have not yet completed
/// `applyPendingFileChanges`.
final _driverPool = Pool(1);

/// A function that returns the path to the SDK summary when invoked.
///
/// Defaults to [_defaultSdkSummaryGenerator].
Expand Down Expand Up @@ -353,14 +377,14 @@ class AnalyzerResolvers implements Resolvers {
await loadPackageConfigUri((await Isolate.packageConfig)!);
var driver = await analysisDriver(uriResolver, _analysisOptions,
await _sdkSummaryGenerator(), loadedConfig);
_resolver = AnalyzerResolver(driver, uriResolver);
_resolver = AnalyzerResolver(driver, _driverPool, uriResolver);
}()));
}

@override
Future<ReleasableResolver> get(BuildStep buildStep) async {
await _ensureInitialized();
return PerActionResolver(_resolver, buildStep);
return PerActionResolver(_resolver, _driverPool, buildStep);
}

/// Must be called between each build.
Expand Down
2 changes: 1 addition & 1 deletion build_resolvers/pubspec.yaml
View file
@@ -1,5 +1,5 @@
name: build_resolvers
version: 2.0.9-dev
version: 2.0.9
description: Resolve Dart code in a Builder
repository: https://github.com/dart-lang/build/tree/master/build_resolvers

Expand Down

0 comments on commit 7a384f3

Please sign in to comment.