Call dart::bin::Platform::Initialize during Dart VM startup#182689
Conversation
There was a problem hiding this comment.
Code Review
The pull request introduces platform-specific initialization for the Dart VM during startup, specifically configuring signal handlers. This change addresses a known issue related to process handling in Dart. A new test file process_test.dart has been added to cover the Process.start functionality with a missing executable, ensuring robustness. The BUILD.gn file has been updated to include this new test. The changes align with the Flutter style guide's emphasis on testing and correctness.
| if (!dart::bin::Platform::Initialize(false)) { | ||
| FML_LOG(FATAL) << "Dart platform-specific initialization failed"; | ||
| } |
There was a problem hiding this comment.
The dart::bin::Platform::Initialize(false) call is crucial for platform-specific setup, such as signal handlers, addressing the issue mentioned in the PR description. The FML_LOG(FATAL) ensures that if this critical initialization fails, the application terminates gracefully with an informative message.
| "platform_isolate_test.dart", | ||
| "platform_isolate_shutdown_test.dart", | ||
| "plugin_utilities_test.dart", | ||
| "process_test.dart", |
There was a problem hiding this comment.
Adding process_test.dart to the tests list in BUILD.gn is correct and ensures the new test is included in the build process. This adheres to the repository's style guide regarding testing (Repository Style Guide, line 11).
References
- Code should be tested and follow the guidance described in the writing effective tests guide and the running and writing tests guide. (link)
| test('start process with missing executable', () async { | ||
| Future<Process> proc = Process.start('nonexistent-executable', []); | ||
| expect(() async => await proc, throwsA(isA<ProcessException>())); | ||
| }); |
There was a problem hiding this comment.
This test effectively verifies the behavior of Process.start when attempting to execute a nonexistent program, correctly expecting a ProcessException. This is a good edge case test and improves the robustness of the Dart VM's process handling. This aligns with the repository's emphasis on writing effective tests (Repository Style Guide, line 11).
References
- Code should be tested and follow the guidance described in the writing effective tests guide and the running and writing tests guide. (link)
This does platform-specific setup for Dart (such as configuring signal handlers) Fixes flutter#182436 See dart-lang/sdk#62679 See dart-lang/sdk#62680
1e66204 to
bddd7d5
Compare
|
DBC It looks like there could be some fighting and/or disagreement between how Flutter sets up signal handlers and how Dart does. In particular on Windows compare these two areas: Not guarded Guarded by flutter/engine/src/flutter/fml/backtrace.cc Line 129 in 5329e94 Didn't look into any differences on the other platforms, but wanted to raise the flag that there are potentially behavior changes here that don't have good test coverage. |
|
cc @mattkae @knopp in case you have any concerns with the Windows changes above: #182689 (comment) |
…182689) This does platform-specific setup for Dart (such as configuring signal handlers) Fixes flutter#182436 See dart-lang/sdk#62679 See dart-lang/sdk#62680
Also includes: Call dart::bin::Platform::Initialize during Dart VM startup (flutter/flutter#182689)
…182689) This does platform-specific setup for Dart (such as configuring signal handlers) Fixes flutter#182436 See dart-lang/sdk#62679 See dart-lang/sdk#62680
…182689) This does platform-specific setup for Dart (such as configuring signal handlers) Fixes flutter#182436 See dart-lang/sdk#62679 See dart-lang/sdk#62680
This does platform-specific setup for Dart (such as configuring signal handlers)
Fixes #182436
See dart-lang/sdk#62679
See dart-lang/sdk#62680