possible concurrent modification error in finalizeExports

[sigmundch]

@jakemac53 run into this exception, which seems related to the current support for exports:

Concurrent modification during iteration: _LinkedHashMap len:121.
#0      _CompactIterator.moveNext (dart:collection-patch/compact_hash.dart:431)
#1      _HashVMBase&MapMixin&&_LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:354)
#2      DillLoader.finalizeExports (package:front_end/src/fasta/dill/dill_loader.dart:62)
#3      DillTarget.buildOutlines (package:front_end/src/fasta/dill/dill_target.dart:54)
<asynchronous suspension>
#4      generateKernelInternal (package:front_end/src/kernel_generator_impl.dart:91)
<asynchronous suspension>
#5      generateKernel.<anonymous closure> (package:front_end/src/kernel_generator_impl.dart:31)
<asynchronous suspension>
#6      CompilerContext.runInContext.<anonymous closure> (package:front_end/src/fasta/compiler_context.dart:89)
#7      _rootRun (dart:async/zone.dart:1120)
#8      _CustomZone.run (dart:async/zone.dart:1001)
#9      runZoned (dart:async/zone.dart:1467)
#10     CompilerContext.runInContext (package:front_end/src/fasta/compiler_context.dart:89)
#11     CompilerContext.runWithOptions (package:front_end/src/fasta/compiler_context.dart:96)
#12     generateKernel (package:front_end/src/kernel_generator_impl.dart:30)
<asynchronous suspension>
#13     summaryFor (package:front_end/summary_generator.dart:40)
<asynchronous suspension>

I know this is currently in flux, but it's possible that this issue exists both in the old and new version of the code. @jakemac53 - what commit of the sdk did you use when you got this error?

I don't have at this time a small repro, but I can work on one if we need one.

[sigmundch]

/cc @peter-ahe-google @scheglov who both have been doing work recently in this part of the code

[scheglov]

Yes, I saw this too.
One of the motives for 234c4a7 was to fix this.

[peter-ahe-google]

I think this is the problem with the tree-shaker that Konstantin mentioned in #30448.

I can't see any other way of getting a concurrent modification to DillLoader.builders.

[sigmundch]

@jakemac53 is running this in an environment without the tree-shaker though.

[sigmundch]

I'll try to follow up with Jake to create a repro, I'm reopening while we investigate

[scheglov]

I think this happens when there is reexport. Not 100% sure.

You reexport something reexported from somewhere else.
A declares a.dart, b.dart exports a.dart, c.dart exports b.dart, and there d.dart imports c.dart.
So, we attempt to add new builders while processing builders.

[peter-ahe-google]

@scheglov I also think that is what is happening. What I can't figure out is why we add a new builder.

This happens in DillLoader, and it happens at a point where all builders should have been created.

If we can't get a repro, I have some ideas for some instrumentation I can add. However, it is also possible that it is fixed now that we've landed Konstantin's export changes again. If that's the case, my only regret is that we may not get a regression test.

[sigmundch]

The issue triggers when trying to do an export from one .dill file of a library that comes from a different .dill file. Turns out you can trigger this even without a double export.

Here is the repro:

import 'package:async_helper/async_helper.dart' show asyncTest;

import 'package:front_end/src/testing/compiler_common.dart';
import 'package:front_end/front_end.dart';

_onError(e) => (e) => throw '${e.severity}: ${e.message}';

main() {
  asyncTest(() async {
    var sources = <String, dynamic>{
      'a.dart': 'class A  {}',
    };
    var summary = await summarize(['a.dart'], sources,
        options: new CompilerOptions()..onError = _onError);

    sources = <String, dynamic>{
      'b.dart': 'export "a.dart";',
      'a.dill': summary,
    };

    summary = await summarize(['b.dart'], sources,
        inputSummaries: ['a.dill'],
        options: new CompilerOptions()..onError = _onError);

    sources = <String, dynamic>{
      'c.dart': 'import "b.dart"; class B extends A {}',
      'b.dill': summary,
    };

    await compileUnit(['c.dart'], sources,
        inputSummaries: ['b.dill'],
        options: new CompilerOptions()..onError = _onError);
  });
}

I see this failing both before and after @scheglov changes.

[peter-ahe-google]

This CL diagnoses the internal problem that leads to this CL 3010963002.

[jakemac53]

friendly ping, still seeing this issue on 2.0.0-dev.4.0

[peter-ahe-google]

I think the root cause of this is that the source loader may attempt to modify its map of builders when reading a dill-backed library it hasn't seen before.

This is currently number three on my personal task list.

[peter-ahe-google]

I'm on vacation next week, but this it now at the top of my task list.

[jakemac53]

Great, thanks for the update!

[vsmenon]

Any update on this? My understanding is that the bazel plumbing for kernel (and thus ddc-kernel) is blocked on this.

[peter-ahe-google]

Sorry, I had to extend my vacation for another week. Good news is that I've been thinking about how to solve the problem while away on vacation, and I think I know how to fix it.

[peter-ahe-google]

The problem I've been thinking about during my vacation was unrelated.

The problem in this bug report is that a.dart is marked external as of 2289ce1. That implies that you need to load a.dill as well to provide the implementation of it.

So this program works:

import 'package:async_helper/async_helper.dart' show asyncTest;

import 'package:front_end/src/testing/compiler_common.dart';
import 'package:front_end/front_end.dart';

_onError(e) => (e) => throw '${e.severity}: ${e.message}';

main() {
  asyncTest(() async {
    var sources = <String, dynamic>{
      'a.dart': 'class A  {}',
    };
    var aSummary = await summarize(['a.dart'], sources,
        options: new CompilerOptions()
          ..verbose = true
          ..onError = _onError);

    sources = <String, dynamic>{
      'b.dart': 'export "a.dart";',
      'a.dill': aSummary,
    };

    var bSummary = await summarize(['b.dart'], sources,
        inputSummaries: ['a.dill'],
        options: new CompilerOptions()
          ..verbose = true
          ..onError = _onError);

    sources = <String, dynamic>{
      'c.dart': 'import "b.dart"; class B extends A {}',
      'a.dill': aSummary,
      'b.dill': bSummary,
    };

    await compileUnit(['c.dart'], sources,
        inputSummaries: [

Key difference:

          'a.dill',

          'b.dill',
        ],
        options: new CompilerOptions()
          ..verbose = true
          ..onError = _onError);
  });
}

[peter-ahe-google]

I think the resolution of this bug is outside the front-end. I think bazel needs to provide all summaries, not just the result of the previous compilation.

[jakemac53]

I have tried supplying all transitive summaries in bazel as well, and ran into the same issue.

[peter-ahe-google]

Here is a CL that avoids concurrent modifications and throws an exception with a bit more information: https://dart-review.googlesource.com/22620

[jakemac53]

Apologies, I actually had a bug in the code that was supposed to provide transitive summaries so it wasn't, I retried with a fix and now it seems to work, thanks!

[peter-ahe-google]

No need to apologize. My only regret is that I confused this with a more obscure concurrent modification error instead of trying the repro directly. Hopefully, the internal errors of the above CL will make it easier to debug another time.