[native_assets_builder] Lock the build directory #1384
Conversation
PR HealthBreaking changes ✔️Details
Changelog Entry ✔️Details
Changes to files need to be accounted for in their respective changelogs. API leaks ✔️DetailsThe following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
License Headers ✔️Details
All source files should start with a license header. Unrelated files missing license headers
Package publish validation ✔️Details
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
|
@lrhn Could you please review |
|
@mkustermann @lrhn Any suggestions on how to address exclusivity within the Dart process? Within an isolate we can achieve this with a global variable. But within an isolate group we'd need |
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
|
The zones are fine. Everything happens in the same zone where |
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
pkgs/native_assets_builder/lib/src/build_runner/build_runner.dart
Outdated
Show resolved
Hide resolved
|
Greatly appreciated @mkustermann ! |
| Logger? logger, | ||
| }) async { | ||
| const lockFileName = '.lock'; | ||
| final lockFile = File.fromUri(directory.uri.resolve(lockFileName)); |
There was a problem hiding this comment.
Either use package:path, or make your own join function:
static File _fileInDir(Directory path, String filename) {
var separator = Platform.pathSeparator;
var dirPath = path.path;
if (path.endsWith(separator)) separator = '';
return File('$dirPath$separator$filename');
}Creating a URI from a Directory path (including converting all platform path separators to / on Windows), do one resolve with a file name, and the converting that back to a file path, is far too much work for what it does.
It may be "easy to write" (you don't have to worry about using the right path separator, or having multiple of them), but it's introducing a concept (Uri) that has no place in the code. That makes the code more complicated than necessary.
| ); | ||
| printed = true; | ||
| } | ||
| await Future<void>.delayed(const Duration(milliseconds: 50)); |
There was a problem hiding this comment.
Why this wait?
There's probably a good reason. Document it and why the magic number was chosen.
If I understand it, it's just a delay before retrying. Reasonable to have one. Is 50 ms a little short? Or long? (I don't know! Depends on why it could fail to get the lock, rather than just block until it has the lock. If there is no timeout, could we use a blocking async lock, if one exists?)
| Logger? logger, | ||
| }) async { | ||
| if (!await file.exists()) await file.create(recursive: true); | ||
| final randomAccessFile = await file.open(mode: FileMode.write); |
There was a problem hiding this comment.
If Windows does not allow another file descriptor to write to a locked file, and FileMode.write says "The file is overwritten if it already exists. The file is created if it does not already exist.", shouldn't this fail if the file is already locked? At least on Windows?
(If not, why not?)
There was a problem hiding this comment.
This case is covered by multiple tests. They explicitly lock the file, and then spawn a new process that executes exactly this code. I believe opening the file in write mode (which gives you a file handle) is fine. It's the write operation with such file handle that will fail. (You need a file handle to do the locking call to begin with, so it would be weird if getting the file handle would fail.)
This makes the native assets builder lock the build directories so that it can be invoked concurrently from multiple `dart` and `flutter` processes. Closes: #1319 The implementation of the locking is in `package:native_assets_cli/locking.dart` contains `runUnderDirectoryLock`, so that we can reuse it for #1379. (#1379 Requires more changes though, it needs to also give users access to a shared directory via the `BuildConfig`.)
This makes the native assets builder lock the build directories so that it can be invoked concurrently from multiple
dartandflutterprocesses.Closes: #1319
The implementation of the locking is in
package:native_assets_cli/locking.dartcontainsrunUnderDirectoryLock, so that we can reuse it for #1379. (#1379 Requires more changes though, it needs to also give users access to a shared directory via theBuildConfig.)