Make the startup lock message print to stderr.

[gspencergoog]

Description

This changes the "Waiting for another flutter command to release the startup lock..." message output so that it appears on stderr instead of stdout. When it appears on stdout, it can mess up collection of the output. For instance, if you run flutter --version --machine and you're expecting JSON output, then you'll get non-JSON output even though the lock is released and you eventually would get what you're asking for.

Tests

• Added an integration test that runs a separate process that locks the lock file.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[jonahwilliams]

What about an integration test? In theory you can start two flutter commands immediately and you have a pretty good chance of hitting the lock message on one. Not sure if that would be reliable enough.

If that doesn't work, a test that uses the real file system would be ideal

[gspencergoog]

I'll see if I can make one with the real filesystem, I feel like starting two commands might be flaky.

I think I can just open the lock file and lock it before calling lock and maybe that'll work.

[gspencergoog]

OK, I've tried a couple of approaches, but neither quite works.

I tried to make a test with the real filesystem, but the problem there is that locks are per process, so even running it in an isolate won't result in the lock being denied: the process already has a lock on that file, so it says "Sure, you can have the lock: you have it already".

I then tried to run an external process that would lock a file, and while that does lock the file, it means I need to know where the Dart executable is, so that I can be assured that the lock is the same kind of lock (e.g. the obvious alternative of flock doesn't work).

Here's what the test looks like (note the absolute path to Dart):

    testWithoutContext('should log a message to stderr when lock is not acquired', () async {
      final String oldRoot = Cache.flutterRoot;
      final FileSystem fileSystem = LocalFileSystem.test(signals: Signals.test());
      final Directory tempDir = fileSystem.systemTempDirectory.createTempSync('cache_test.');
      final FakeLogger logger = FakeLogger();
      try {
        Cache.flutterRoot = tempDir.absolute.path;
        final Cache cache = Cache.test(
            fileSystem: fileSystem, processManager: FakeProcessManager.any(), logger: logger);
        final File cacheFile = fileSystem.file(fileSystem.path.join(Cache.flutterRoot, 'bin', 'cache', 'lockfile'))
          ..createSync(recursive: true);
        final File script = fileSystem.file(fileSystem.path.join(Cache.flutterRoot, 'bin', 'cache', 'test_lock.dart'));
        script.writeAsStringSync(r'''
import 'dart:async';
import 'dart:io';

Future<void> main(List<String> args) async {
  File file = File(args[0]);
  RandomAccessFile lock = file.openSync(mode: FileMode.write);
  lock.lockSync();
  await Future<void>.delayed(const Duration(milliseconds: 1000));
  exit(0);
}
''');
        final Process process = await const LocalProcessManager().start(<String>['/home/gspencer/code/flutter/bin/cache/dart-sdk/bin/dart', script.absolute.path, cacheFile.absolute.path]);
        await Future<void>.delayed(const Duration(milliseconds: 500));
        await cache.lock();
        process.kill(io.ProcessSignal.sigkill);
      } finally {
        tempDir.deleteSync(recursive: true);
        Cache.flutterRoot = oldRoot;
      }
      expect(logger.errors.single, equals('Waiting for another flutter command to release the startup lock...'));
      expect(logger.status, isEmpty);
    });

Do you know of a good way to find out where the Dart executable is in a tools test?

[jonahwilliams]

Something like https://github.com/flutter/flutter/blob/master/packages/flutter_tools/test/integration.shard/tool_backend_test.dart#L14 ?

[gspencergoog]

Yes, that should work. I've added the test: hopefully it'll work on the server where FLUTTER_ROOT is set. Running it locally fails with:

Bad state: Could not determine flutter_tools/ path from script URL (file:///tmp/dart_test.VRABED/test.dart_1.dill); consider setting FLUTTER_ROOT explicitly.

[christopherfujino]

on Windows:

21:53 +47 ~2 -1: test\integration.shard\cache_test.dart: Cache.lock should log a message to stderr when lock is not acquired [E]                                                                       
  Bad state: No element
  dart:core                                      List.single
  test\integration.shard\cache_test.dart 128:28  main.<fn>.<fn>

[gspencergoog]

Yeah, I was looking at that. It seems like Windows might be flakier than the other platforms when it comes to locking performance. If I step through in the debugger, the test passes, but timing prevents it from passing when run normally. I might just disable this test on Windows: the test itself is just to make sure we're passing things to stderr, and we don't necessarily need to verify that on all platforms (although it would be nice).

[jonahwilliams]

Please use the BufferLogger class instead

[gspencergoog]

Ahh, that's what it's called. I was looking for something like that (expected it to be called "Fake" something). OK.

[gspencergoog]

OK, Fixed. PTAL

[christopherfujino]

@gspencergoog bad merge?

[gspencergoog]

Yep. Fixed.

[gspencergoog]

@jonahwilliams I think I resolved everything, please take another look.

[jonahwilliams]

LGTM!