Require analyzer 3.4.1, prepare for future breaking changes.

[scheglov]

In analyzer 4.0.0 we will stop processing changeFile() immediately, and instead will do it between other async requests. But this will invalidate the session, so applyPendingFileChanges() is required before invoking any AnalysisSession API.

Also, linking elements will become an async operation to support future work on macros (which require async talking to the macro running isolates).

The internal presubmit looks green.

[jakemac53]

Seeing a lot of InconsistentAnalysisException: Requested result might be inconsistent with previously returned results in the community tests, cc @simolus3 do you know if that builder is doing any special stuff to try and work around those errors today, which might not work after this change due to timing changes?

[scheglov]

It looks that the difference in analyzer 3.4.0 is that we now check AnalysisSession consistency not only before performing an asynchronous operation, like getLibraryByUri, but also after it. So, if there were any changeFile() invocations during the async computation, the result is considered to be inconsistent.

[simolus3]

In my builder, I think there's some logic related to getting a resolved AST that still uses the analyzer APIs in a retry loop instead of the new method from the Resolver. So I'll take this as an early warning and fix my builder :D

[simolus3]

Actually it looks like the exceptions are more severe , they're coming from buildStep.resolver.libraryFor. I'll investigate.

[eernstg]

So, if there were any changeFile() invocations during the async computation, the result is considered to be inconsistent.

@jakemac53, is this intended to mean that the session is considered inconsistent if any changeFile() invocations occurred? Or would it have to be a changeFile() invocation that changes a file which is relevant to the current code generation process?

This could explain how the interleaved execution of reflectable code generation on tests gives rise to these inconsistency exceptions: We're generating code for a few hundred tests, and the code generation is interleaved, so it is definitely possible that one program's generated code is stored on disk during the time where some other program's code generation is ongoing.

[simolus3]

(This comment is pretty much saying the same thing as eernstg, I was just a few minutes late :D But I think there's no easy fix as all builders share the same driver-bound analysis session)

I'm convinced these errors aren't just me doing anything weird in the community tests, I think they should also happen for every other source_gen generator running on enough sources (and the linked issue also seems to point towards this).

What I think might be causing this issue is that, in a typical build, there are lots of concurrent calls to getLibraryByUri used by build steps resolving their main input independently. Previously this wasn't a problem as the consistency check was done synchronously and all the async operations pending could run concurrently. But now the analysis session isn't allowed to change during the async operation either, so this is starting to be a problem when we resolve two libraries at once.

I was able to fix this by wrapping every async analyzer API in a Pool(1) (see the attached patch for a hacky example), but I hope serializing resolve calls isn't the best solution as I imagine it would slow down builds.

diff --git a/build_resolvers/lib/src/resolver.dart b/build_resolvers/lib/src/resolver.dart
index 099bf532..ecd04bcf 100644
--- a/build_resolvers/lib/src/resolver.dart
+++ b/build_resolvers/lib/src/resolver.dart
@@ -117,7 +117,7 @@ class PerActionResolver implements ReleasableResolver {
   // 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);
+  Pool get _resolvePool => _delegate._driverLock;
   Future<void> _resolveIfNecessary(AssetId id, {required bool transitive}) =>
       _resolvePool.withResource(() async {
         if (!_entryPoints.contains(id)) {
@@ -147,6 +147,8 @@ class AnalyzerResolver implements ReleasableResolver {
   final BuildAssetUriResolver _uriResolver;
   final AnalysisDriverForPackageBuild _driver;
 
+  final Pool _driverLock = Pool(1);
+
   AnalyzerResolver(this._driver, this._uriResolver);
 
   @override
@@ -160,7 +162,6 @@ class AnalyzerResolver implements ReleasableResolver {
 
   @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
@@ -170,11 +171,14 @@ class AnalyzerResolver implements ReleasableResolver {
     var path = library.source.fullName;
 
     if (resolve) {
-      return (await session.getResolvedLibrary(path) as ResolvedLibraryResult)
-          .getElementDeclaration(element)
-          ?.node;
+      final resolved = await _driverLock.withResource(
+              () => _driver.currentSession.getResolvedLibrary(path))
+          as ResolvedLibraryResult;
+
+      return resolved.getElementDeclaration(element)?.node;
     } else {
-      return (session.getParsedLibrary(path) as ParsedLibraryResult)
+      return (_driver.currentSession.getParsedLibrary(path)
+              as ParsedLibraryResult)
           .getElementDeclaration(element)
           ?.node;
     }
@@ -210,7 +214,8 @@ class AnalyzerResolver implements ReleasableResolver {
       throw NonLibraryAssetException(assetId);
     }
 
-    final library = await _driver.currentSession.getLibraryByUri(uri.toString())
+    final library = await _driverLock.withResource(
+            () => _driver.currentSession.getLibraryByUri(uri.toString()))
         as LibraryElementResult;
     if (!allowSyntaxErrors) {
       final errors = await _syntacticErrorsFor(library.element);
@@ -243,8 +248,8 @@ class AnalyzerResolver implements ReleasableResolver {
     final relevantResults = <ErrorsResult>[];
 
     for (final path in paths) {
-      final result =
-          await _driver.currentSession.getErrors(path) as ErrorsResult;
+      final result = await _driverLock.withResource(
+          () => _driver.currentSession.getErrors(path)) as ErrorsResult;
       if (result.errors
           .any((error) => error.errorCode.type == ErrorType.SYNTACTIC_ERROR)) {
         relevantResults.add(result);

[jakemac53]

cc @scheglov do you have any suggestions here?

[scheglov]

See also dart-lang/sdk#48658

[jakemac53]

LGTM, not sure what is up with the markdown job here, will land on red for right now