Skip to content

Fix race conditions during test load - #2761

Merged
natebosch merged 6 commits into
masterfrom
fix-load-suite-close-deadlock
Sep 19, 2026
Merged

Fix race conditions during test load#2761
natebosch merged 6 commits into
masterfrom
fix-load-suite-close-deadlock

Conversation

@natebosch

@natebosch natebosch commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Engine._addLoadSuite awaits LoadSuite.suite, whose completer is only
ever completed from inside the load test's body: either when the body
finishes loading the suite, or by the onComplete handler the body
registers. If the load test is closed before its body runs at all,
neither happens and nothing ever completes the suite.

That stalls the task Engine.run added for the suite, so it never
releases its resource from _runPool, so _runPool.close() never
completes, so Engine.close() and in turn Runner.close() hang
forever.

Today this is masked in the runner: the signal handler cancels its
signal subscription immediately, so the VM exits once nothing else
holds the event loop even though close() never completed. Anything
that keeps the isolate alive past that point turns it into a real hang.

Hold onto the completer in LoadSuite and add completeWithoutSuite,
which completes it with a null suite if nothing else has. Call it once
the load test has finished running, from both Engine._addLoadSuite
and LoadSuite.getSuite. It is a no-op in every normal path, since the
body holds an outstanding callback until it completes the completer.

Skip running the load test when the engine is already closed. Nothing
needs to be loaded for a closed engine, and completeWithoutSuite still
unblocks anything waiting on the suite.

@natebosch
Fix a hang when a suite is closed before it starts loading
0ecb402 
`Engine._addLoadSuite` awaits `LoadSuite.suite`, whose completer is only
ever completed from inside the load test's body: either when the body
finishes loading the suite, or by the `onComplete` handler the body
registers. If the load test is closed before its body runs at all,
neither happens and nothing ever completes the suite.

That stalls the task `Engine.run` added for the suite, so it never
releases its resource from `_runPool`, so `_runPool.close()` never
completes, so `Engine.close()` and in turn `Runner.close()` hang
forever.

Today this is masked in the runner: the signal handler cancels its
signal subscription immediately, so the VM exits once nothing else
holds the event loop even though `close()` never completed. Anything
that keeps the isolate alive past that point turns it into a real hang.

Hold onto the completer in `LoadSuite` and add `completeWithoutSuite`,
which completes it with a null suite if nothing else has. Call it once
the load test has finished running, from both `Engine._addLoadSuite`
and `LoadSuite.getSuite`. It is a no-op in every normal path, since the
body holds an outstanding callback until it completes the completer.
@github-actions

github-actions Bot commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

This check can be disabled by tagging the PR with skip-changelog-check.

@natebosch
Merge branch 'master' into fix-load-suite-close-deadlock
427c665
@natebosch
Don't run a load test that has already been closed
c802dc2 
`Engine._addLoadSuite` schedules the load test with
`Future.microtask(liveTest.run)`. If the engine is closed before that
microtask runs, the live test has already been closed, and
`LiveTestController.run` throws `LiveTest.run() may not be called for a
closed test`. That surfaces as the runner's "unexpected error, please
file an issue" message.

This is the same shutdown race as the hang fixed in the previous
commit, landing one event loop turn earlier: there the load test had
started and was closed before its body ran, here it is closed before it
is run at all.

Skip running it when the engine is already closed. Nothing needs to be
loaded for a closed engine, and `completeWithoutSuite` still unblocks
anything waiting on the suite.

Also reword the changelog entries: the load test has started by the
point the hang happens, it just hasn't reached the body yet, so "in the
queue to be loaded" was misleading.
@natebosch
Rename and style tweaks
eb8524a
@natebosch
Expand doc comment
7179fb1
@natebosch natebosch changed the title Fix a hang when a suite is closed before it starts loading Fix race conditions during test load Sep 19, 2026
@natebosch
natebosch marked this pull request as ready for review September 19, 2026 00:14
@natebosch
natebosch requested a review from a team as a code owner September 19, 2026 00:14
brianquinlan
brianquinlan approved these changes Sep 19, 2026
View reviewed changes
Comment thread pkgs/test_core/lib/src/runner/engine.dart
// Schedule a microtask to ensure that [onTestStarted] fires before the
// first [LiveTest.onStateChange] event.
await Future.microtask(liveTest.run);
await null;

@brianquinlan brianquinlan Sep 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await null schedules a microtask?

@natebosch natebosch Sep 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, any await waits at least a microtask. It's not a super widespread pattern because it's generally a code smell but IMO await null; is less ugly than await Future.microtask(() {}); with an empty function literal which is the more explicit way to express this.

The old pattern was handy when we could use a function tear-off but it's not a simple tear-off after this change.

@natebosch
natebosch merged commit 48c3fdf into master Sep 19, 2026
71 checks passed
@natebosch
natebosch deleted the fix-load-suite-close-deadlock branch September 19, 2026 01:52
@natebosch natebosch mentioned this pull request Sep 19, 2026
Merged
natebosch added a commit that referenced this pull request Sep 21, 2026
@natebosch
Fix race conditions during test load (#2761)
2835540 
`Engine._addLoadSuite` awaits `LoadSuite.suite`, whose completer is only
ever completed from inside the load test's body: either when the body
finishes loading the suite, or by the `onComplete` handler the body
registers. If the load test is closed before its body runs at all,
neither happens and nothing ever completes the suite.

That stalls the task `Engine.run` added for the suite, so it never
releases its resource from `_runPool`, so `_runPool.close()` never
completes, so `Engine.close()` and in turn `Runner.close()` hang
forever.

Today this is masked in the runner: the signal handler cancels its
signal subscription immediately, so the VM exits once nothing else
holds the event loop even though `close()` never completed. Anything
that keeps the isolate alive past that point turns it into a real hang.

Hold onto the completer in `LoadSuite` and add `completeWithoutSuite`,
which completes it with a null suite if nothing else has. Call it once
the load test has finished running, from both `Engine._addLoadSuite`
and `LoadSuite.getSuite`. It is a no-op in every normal path, since the
body holds an outstanding callback until it completes the completer.

Skip running the load test when the engine is already closed. Nothing
needs to be loaded for a closed engine, and `completeWithoutSuite` still
unblocks anything waiting on the suite.
copybara-service Bot pushed a commit to dart-lang/sdk that referenced this pull request Sep 22, 2026
@natebosch dart-scoped@luci-project-accounts.iam.gserviceaccount.com
[deps] rev dartdoc, ecosystem, http, native, shelf, test, tools
aaa7b10 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/df2f2cd..c68debc):
  c68debcb  Mon Sep 21 11:07:33 2026 +0200  Sarah Zakarias  Adapt the {@example} code fence to the example content (dart-lang/dartdoc#4285)

ecosystem (https://github.com/dart-lang/ecosystem/compare/1bdafd8..16dca7a):
  16dca7a  Mon Sep 21 08:04:10 2026 -0700  Kevin Moore  fix(firehose): keep dart_apitool crash details in logs and sanitize breaking table row (dart-lang/ecosystem#461)

http (https://github.com/dart-lang/http/compare/35769ba..d8e8e9c):
  d8e8e9c  Mon Sep 21 18:26:27 2026 +0900  DubaiChewyCookie  fix(http2): bound peer-controlled streams and headers (dart-lang/http#1958)
  286037b  Thu Sep 17 17:12:02 2026 -0700  Brian Quinlan  chore(http2): prepare to release 3.1.0 (dart-lang/http#1987)
  a067c94  Wed Sep 16 15:57:52 2026 -0700  Brian Quinlan  fix(cupertino_http): possible URLRequest memory leaks (dart-lang/http#1989)
  0d15438  Thu Sep 17 00:43:43 2026 +0300  Ellet  chore(ok_http): Built-in Kotlin and resolve AGP 9 warnings (dart-lang/http#1973)

native (https://github.com/dart-lang/native/compare/a88aaaf..f628b26):
  f628b26f0  Mon Sep 21 23:21:05 2026 +0200  Daco Harkes  [hooks] Add unit tests for `ProtocolExtension` defaults and `build`/`link` error paths (dart-lang/native#3668)
  474a93f39  Mon Sep 21 08:01:13 2026 +0500  Muhammad Anas Naveed  [jni][jnigen] Update Groovy Gradle property assignments (dart-lang/native#3678)
  4434b7f3c  Mon Sep 21 10:31:09 2026 +1000  Liam Appelbe  [jni] Ignore coverage of generated files (dart-lang/native#3665)
  b66dfdeae  Mon Sep 21 10:27:27 2026 +1000  Liam Appelbe  [objective_c] Use `objc_retainAutoreleasedReturnValue` (dart-lang/native#3664)
  541121b4b  Fri Sep 18 17:30:03 2026 +0200  Daco Harkes  [record_use] Add status and coverage badges to `README.md` (dart-lang/native#3675)
  b97663824  Fri Sep 18 17:25:10 2026 +0200  Daco Harkes  [hooks] Prepare `prebuilt_assets_example` v0.1.2 for release (dart-lang/native#3672)
  876145f0c  Fri Sep 18 15:23:34 2026 +0200  Daco Harkes  [hooks] Use `prebuilt/` instead of `assets/` in `prebuilt_assets_example` (dart-lang/native#3671)
  73bd0b7b3  Fri Sep 18 12:58:36 2026 +0200  Daco Harkes  [hooks] Prepare `prebuilt_assets_example` v0.1.1 for release (dart-lang/native#3669)
  4e0f1983f  Fri Sep 18 11:40:30 2026 +0200  Daco Harkes  [hooks] Add example for bundling prebuilt assets in `pub publish` (dart-lang/native#3637)
  22d7cf8da  Fri Sep 18 09:28:25 2026 +0200  Daco Harkes  [native_test_helpers] Add unit tests for `yamlToJson` and `findPackageRoot` (dart-lang/native#3662)
  74e631a8b  Fri Sep 18 09:28:25 2026 +0200  Daco Harkes  [hooks] Add unit tests for `link` API and `MetadataAsset` (dart-lang/native#3661)
  3428cb968  Fri Sep 18 09:28:24 2026 +0200  Daco Harkes  [data_assets] Add unit tests for `LinkOutput` data asset extensions (dart-lang/native#3660)
  0e4eea612  Fri Sep 18 09:28:24 2026 +0200  Daco Harkes  [code_assets] Add unit tests for `testCodeBuildHook` and `CCompilerConfig` (dart-lang/native#3659)
  dc44c5383  Fri Sep 18 09:28:24 2026 +0200  Daco Harkes  [ffi] Add unit tests for `Arena` error handling and `malloc.free` (dart-lang/native#3658)
  1e1e2fb8a  Fri Sep 18 09:28:23 2026 +0200  Daco Harkes  [native_toolchain_c] Default `linkerOptions` in `CLinker.run` and add `CLibrary` tests (dart-lang/native#3657)
  f3c066006  Fri Sep 18 09:28:23 2026 +0200  Daco Harkes  [data_assets] [hooks] [hooks_runner] [record_use] Add coverage ignore comments for debug methods (dart-lang/native#3656)
  59cd44422  Fri Sep 18 09:28:22 2026 +0200  Daco Harkes  [infra] Clean coverage directory and enable `--check-ignore` in `tool/ci.dart` (dart-lang/native#3655)
  b859d5fa8  Fri Sep 18 09:35:46 2026 +0500  Muhammad Anas Naveed  [ffigen] Default compiler options for C++ (dart-lang/native#3645)
  eb6ea1e56  Thu Sep 17 09:28:38 2026 +1000  Liam Appelbe  [ffigen][jnigen] More docs (dart-lang/native#3649)
  da786131c  Thu Sep 17 09:06:20 2026 +1000  Liam Appelbe  [ffigen][jnigen] YAML migration skills (dart-lang/native#3648)
  fc0491997  Wed Sep 16 15:37:19 2026 -0700  Brian Quinlan  [objective_c] don't use autorelease for `toNSDate` (dart-lang/native#3650)
  f7f9a4556  Wed Sep 16 17:32:47 2026 +0200  Daco Harkes  [code_assets] [hooks_runner] [native_toolchain_c] Prepare for release (dart-lang/native#3642)
  1c60c437c  Wed Sep 16 13:40:31 2026 +0200  Daco Harkes  [hooks_runner] Restore non-breaking `KernelAssetPath.toJson` and add `toJsonForTarget` (dart-lang/native#3643)
  7667162b3  Wed Sep 16 11:46:18 2026 +0200  Daco Harkes  [infra] Use `dependency_validator` in `tool/ci.dart` (dart-lang/native#3646)
  f6d153d01  Tue Sep 15 19:32:08 2026 -0700  Brian Quinlan  [objective_c] Fix potential leaks in converts (dart-lang/native#3647)
  cb2c62331  Wed Sep 16 11:32:42 2026 +1000  Liam Appelbe  [ffigen] Remove YAML from readmes (dart-lang/native#3633)
  22e835728  Wed Sep 16 09:49:02 2026 +1000  Liam Appelbe  [jnigen] Remove yaml from readme (dart-lang/native#3634)
  eb444eee2  Wed Sep 16 09:48:04 2026 +1000  Liam Appelbe  [infra] Add and fix `unawaited_futures` and `discarded_futures` lints (dart-lang/native#3639)
  0f419f0d2  Wed Sep 16 09:47:44 2026 +1000  Liam Appelbe  [infra] Switch from coveralls to codecov (dart-lang/native#3626)

shelf (https://github.com/dart-lang/shelf/compare/f36dd68..e5c8dc6):
  e5c8dc6  Tue Sep 15 18:15:41 2026 -0700  Kevin Moore  fix(shelf_static): ignore suffix ranges selecting zero bytes (dart-lang/shelf#541)

test (https://github.com/dart-lang/test/compare/c80d53d..9946c95):
  9946c955  Mon Sep 21 12:31:54 2026 -0700  Nate Bosch  Fix a race in the frontend server client fake (dart-lang/test#2762)
  48c3fdf6  Fri Sep 18 18:52:52 2026 -0700  Nate Bosch  Fix race conditions during test load (dart-lang/test#2761)
  5d74864b  Fri Sep 18 16:25:57 2026 -0700  Nate Bosch  Remove mentions of Travis (dart-lang/test#2760)
  5c4d9b25  Fri Sep 18 12:49:00 2026 -0700  Nate Bosch  Use consistent *Checks naming for prints (dart-lang/test#2759)
  f927df26  Fri Sep 18 12:15:49 2026 -0700  Kevin Moore  feat(checks): add prints expectations (`#2742`) (dart-lang/test#2743)
  347a5af2  Fri Sep 18 12:09:15 2026 -0700  Nate Bosch  Add an example for every matcher (dart-lang/test#2757)
  6358dbb3  Thu Sep 17 15:10:08 2026 -0700  Nate Bosch  Add an example for every checks expectation, and fix the bugs they exposed (dart-lang/test#2756)
  88202adc  Thu Sep 17 14:13:30 2026 +0200  Sigurd Meldgaard  New flag --shard-by-suite

tools (https://github.com/dart-lang/tools/compare/1b0c3bb..3614ce0):
  3614ce06  Fri Sep 18 10:07:20 2026 -0700  Kevin Moore  chore(unified_analytics): remove dart:mirrors usage in test/event_test.dart (dart-lang/tools#2602)
  7550ce13  Fri Sep 18 07:56:44 2026 -0700  Kevin Moore  chore(markdown): remove dart:mirrors usage in tool/stats_lib.dart (dart-lang/tools#2600)

[email protected]

Change-Id: Ide262779269adab780d314d80c9f82b97a79391f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/552741
Commit-Queue: Nate Bosch <[email protected]>
Reviewed-by: Brian Quinlan <[email protected]>
@kevmoo kevmoo mentioned this pull request Sep 23, 2026
Closed
@natebosch natebosch mentioned this pull request Oct 14, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brianquinlan brianquinlan brianquinlan approved these changes

Assignees

No one assigned

Labels

package:test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@natebosch @brianquinlan