Skip to content

Commit 919d503

Browse files
liamappelbeCommit Queue
authored andcommitted
[vm] Fix deadlock in NativeCallable.listener
Switch all lockers of FfiCallbackMetadata::lock_ in FfiCallbackMetadata from MutexLocker to SafepointMutexLocker. The new test deadlocks on patchset 2, before the fix: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8706690957454603105/+/u/test_results/new_test_failures__logs_ And succeeds on patchset 4, after the fix. I found that switching the MutexLocker in DLRT_GetFfiCallbackMetadata to a SafepointMutexLocker caused other NativeCallable.listener tests to fail, so I've left that one unchanged: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8706685486083640001/+/u/test_results/new_test_failures__logs_ Change-Id: I3e48e05d56806488754751b23db27f208f9d396d Fixes: #61272 TEST=tests/ffi/many_listener_callbacks_test.dart Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/445000 Commit-Queue: Liam Appelbe <[email protected]> Reviewed-by: Slava Egorov <[email protected]>
1 parent af5e3aa commit 919d503

File tree

3 files changed

+93
-3
lines changed

3 files changed

+93
-3
lines changed

runtime/bin/ffi_test/ffi_test_functions.cc

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1332,6 +1332,29 @@ DART_EXPORT void CallFunctionOnNewThreadBlocking(int64_t response_id,
13321332
thread.join();
13331333
}
13341334

1335+
struct RepeatingThread {
1336+
std::thread thread;
1337+
std::atomic<bool> run;
1338+
};
1339+
1340+
DART_EXPORT void* CallFunctionOnNewThreadRepeatedly(void (*fn)(int64_t)) {
1341+
RepeatingThread* repeater = new RepeatingThread();
1342+
repeater->run.store(true);
1343+
repeater->thread = std::thread([fn, repeater]() {
1344+
for (int64_t i = 0; repeater->run.load(); ++i) {
1345+
fn(i);
1346+
}
1347+
});
1348+
return repeater;
1349+
}
1350+
1351+
DART_EXPORT void CallFunctionOnNewThreadStop(void* ptr) {
1352+
RepeatingThread* repeater = reinterpret_cast<RepeatingThread*>(ptr);
1353+
repeater->run.store(false);
1354+
repeater->thread.join();
1355+
delete repeater;
1356+
}
1357+
13351358
#if defined(__linux__)
13361359
struct Data {
13371360
void (*fn)(int64_t, int32_t);

runtime/vm/ffi_callback_metadata.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -251,7 +251,7 @@ FfiCallbackMetadata::Trampoline FfiCallbackMetadata::CreateMetadataEntry(
251251
uword target_entry_point,
252252
uint64_t context,
253253
MetadataEntry** list_head) {
254-
MutexLocker locker(&lock_);
254+
SafepointMutexLocker locker(&lock_);
255255
EnsureFreeListNotEmptyLocked();
256256
ASSERT(free_list_head_ != nullptr);
257257
MetadataEntry* entry = free_list_head_;
@@ -305,7 +305,7 @@ void FfiCallbackMetadata::DeleteCallbackLocked(MetadataEntry* entry) {
305305
}
306306

307307
void FfiCallbackMetadata::DeleteAllCallbacks(MetadataEntry** list_head) {
308-
MutexLocker locker(&lock_);
308+
SafepointMutexLocker locker(&lock_);
309309
for (MetadataEntry* entry = *list_head; entry != nullptr;) {
310310
MetadataEntry* next = entry->list_next();
311311
DeleteCallbackLocked(entry);
@@ -316,7 +316,7 @@ void FfiCallbackMetadata::DeleteAllCallbacks(MetadataEntry** list_head) {
316316

317317
void FfiCallbackMetadata::DeleteCallback(Trampoline trampoline,
318318
MetadataEntry** list_head) {
319-
MutexLocker locker(&lock_);
319+
SafepointMutexLocker locker(&lock_);
320320
auto* entry = MetadataEntryOfTrampoline(trampoline);
321321
ASSERT(entry->metadata()->IsLive());
322322
auto* prev = entry->list_prev_;
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
// Tests that interleaved creation and calling of many NativeCallable.listener
6+
// callbacks does not deadlock.
7+
// Regression test for https://github.com/dart-lang/sdk/issues/61272
8+
//
9+
// VMOptions=
10+
// VMOptions=--use-slow-path
11+
// VMOptions=--use-slow-path --stacktrace-every=100
12+
// VMOptions=--dwarf_stack_traces --no-retain_function_objects --no-retain_code_objects
13+
// VMOptions=--test_il_serialization
14+
// VMOptions=--profiler --profile_vm=true
15+
// VMOptions=--profiler --profile_vm=false
16+
// SharedObjects=ffi_test_functions
17+
18+
import 'dart:async';
19+
import 'dart:ffi';
20+
21+
import 'dylib_utils.dart';
22+
23+
typedef FnStartNativeType = Pointer Function(Pointer);
24+
typedef FnStartType = Pointer Function(Pointer);
25+
typedef FnStopNativeType = Void Function(Pointer);
26+
typedef FnStopType = void Function(Pointer);
27+
28+
late final FnStartType callFunctionOnNewThreadRepeatedly;
29+
late final FnStopType callFunctionOnNewThreadStop;
30+
31+
void main() async {
32+
final ffiTestFunctions = dlopenPlatformSpecific('ffi_test_functions');
33+
callFunctionOnNewThreadRepeatedly = ffiTestFunctions
34+
.lookupFunction<FnStartNativeType, FnStartType>(
35+
'CallFunctionOnNewThreadRepeatedly',
36+
);
37+
callFunctionOnNewThreadStop = ffiTestFunctions
38+
.lookupFunction<FnStopNativeType, FnStopType>(
39+
'CallFunctionOnNewThreadStop',
40+
);
41+
42+
final repeaters = <(Pointer, NativeCallable)>[];
43+
void spawn(void Function(int) callback) {
44+
final listener = NativeCallable<Void Function(Int32)>.listener(callback);
45+
final repeater = callFunctionOnNewThreadRepeatedly(listener.nativeFunction);
46+
repeaters.add((repeater, listener));
47+
}
48+
49+
// Spawn one repeater, wait for it to start running, then spawn 30 more.
50+
final firstIsRunning = Completer<void>();
51+
spawn((int n) {
52+
if (n == 10) firstIsRunning.complete();
53+
});
54+
55+
await firstIsRunning.future;
56+
57+
for (var i = 0; i < 30; ++i) {
58+
spawn((int n) {});
59+
}
60+
61+
await Future.delayed(Duration(seconds: 3));
62+
63+
for (final (repeater, listener) in repeaters) {
64+
callFunctionOnNewThreadStop(repeater);
65+
listener.close();
66+
}
67+
}

0 commit comments

Comments
 (0)