Skip to content

Commit 07f3a26

Browse files
edwinwugoogcopybara-github
edwinwugoog
authored and
copybara-github
committed
Fix test flakiness.
PiperOrigin-RevId: 844654834
1 parent ee108c5 commit 07f3a26

File tree

9 files changed

+117
-62
lines changed

9 files changed

+117
-62
lines changed

‎internal/platform/implementation/apple/Mediums/BLE/GNCBLEL2CAPConnection.m‎

Lines changed: 37 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -119,26 +119,39 @@ - (void)sendData:(NSData *)data completion:(void (^)(BOOL))completion {
119119

120120
- (void)requestDataConnectionWithCompletion:(void (^)(BOOL))completion {
121121
GNCLoggerInfo(@"Sending l2cap packet request data connection");
122-
// TODO b/399815436 - A bug is causing channel has written to the socket but the remote does not
123-
// receive it. Add a delay to make sure the data is written to the socket. Remove the delay once
124-
// the bug is fixed.
125-
dispatch_time_t requestTime =
126-
dispatch_time(DISPATCH_TIME_NOW, kRequestDataConnectionDelayInSeconds * NSEC_PER_SEC);
127-
dispatch_after(requestTime, _selfQueue, ^(void) {
128-
[_requestDataConnectionCondition lock];
129-
NSData *requestDataConnectionPacket =
130-
GNCMGenerateBLEL2CAPPacket(GNCMBLEL2CAPCommandRequestDataConnection, nil);
131-
[_stream sendData:PrefixLengthData(requestDataConnectionPacket)
132-
completionBlock:^(BOOL result){
133122

134-
}];
135-
dispatch_async(_callbackQueue, ^{
136-
NSDate *requestDataConnectionTimeout =
137-
[NSDate dateWithTimeIntervalSinceNow:kRequestDataConnectionTimeoutInSeconds];
138-
BOOL result = [_requestDataConnectionCondition waitUntilDate:requestDataConnectionTimeout];
139-
completion(result);
140-
[_requestDataConnectionCondition unlock];
123+
dispatch_async(_callbackQueue, ^{
124+
[_requestDataConnectionCondition lock];
125+
126+
// TODO b/399815436 - A bug is causing channel has written to the socket but the remote does not
127+
// receive it. Add a delay to make sure the data is written to the socket. Remove the delay once
128+
// the bug is fixed.
129+
dispatch_time_t requestTime =
130+
dispatch_time(DISPATCH_TIME_NOW, kRequestDataConnectionDelayInSeconds * NSEC_PER_SEC);
131+
dispatch_after(requestTime, _selfQueue, ^(void) {
132+
NSData *requestDataConnectionPacket =
133+
GNCMGenerateBLEL2CAPPacket(GNCMBLEL2CAPCommandRequestDataConnection, nil);
134+
[_stream sendData:PrefixLengthData(requestDataConnectionPacket)
135+
completionBlock:^(BOOL result){
136+
}];
141137
});
138+
139+
NSDate *requestDataConnectionTimeout =
140+
[NSDate dateWithTimeIntervalSinceNow:kRequestDataConnectionTimeoutInSeconds];
141+
// Wait until the response is received or timeout.
142+
// Note: We are checking _handledReceivedL2CAPResponseDataConnectionReadyPacket without lock
143+
// protection from writer on _selfQueue, but _requestDataConnectionCondition acts as memory barrier
144+
// when signaled. For robustness, we assume the signal implies the flag is set.
145+
BOOL result = YES;
146+
while (!self.handledReceivedL2CAPResponseDataConnectionReadyPacket) {
147+
if (![_requestDataConnectionCondition waitUntilDate:requestDataConnectionTimeout]) {
148+
result = NO;
149+
break;
150+
}
151+
}
152+
153+
[_requestDataConnectionCondition unlock];
154+
completion(result);
142155
});
143156
}
144157

@@ -297,16 +310,16 @@ - (BOOL)handleL2CAPPacketFromData:(NSData *)data {
297310
break;
298311
}
299312
[_requestDataConnectionCondition lock];
313+
_handledReceivedL2CAPResponseDataConnectionReadyPacket = YES;
314+
_handledOutgoingConnectionL2CAPPacket = YES;
315+
[_requestDataConnectionCondition broadcast];
316+
[_requestDataConnectionCondition unlock];
317+
300318
dispatch_async(_selfQueue, ^{
301319
[_stream sendData:PrefixLengthData(GNCMGenerateBLEFramesIntroductionPacket(_serviceIDHash))
302-
completionBlock:^(BOOL result) {
303-
[_requestDataConnectionCondition broadcast];
304-
[_requestDataConnectionCondition unlock];
320+
completionBlock:^(BOOL result){
305321
}];
306322
});
307-
_handledReceivedL2CAPResponseDataConnectionReadyPacket = YES;
308-
_handledOutgoingConnectionL2CAPPacket =
309-
_handledReceivedL2CAPResponseDataConnectionReadyPacket;
310323

311324
} break;
312325
case GNCMBLEL2CAPCommandRequestDataConnection: {

‎internal/platform/implementation/apple/Mediums/BLE/GNCBLEMedium.m‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ @implementation GNCBLEMedium {
9999
- (instancetype)init {
100100
dispatch_queue_t queue = dispatch_queue_create(kBLEMediumQueueLabel, DISPATCH_QUEUE_SERIAL);
101101
CBCentralManager *centralManager =
102-
[[CBCentralManager alloc] initWithDelegate:self
102+
[[CBCentralManager alloc] initWithDelegate:nil
103103
queue:queue
104104
options:@{CBCentralManagerOptionShowPowerAlertKey : @NO}];
105105
return [self initWithCentralManager:centralManager queue:queue];
@@ -113,12 +113,12 @@ - (instancetype)initWithCentralManager:(id<GNCCentralManager>)centralManager
113113
if (self) {
114114
_queue = queue ?: dispatch_get_main_queue();
115115
_centralManager = centralManager;
116-
_centralManager.centralDelegate = self;
117116
_gattConnectionCompletionHandlers = [NSMutableDictionary dictionary];
118117
_gattDisconnectionHandlers = [NSMutableDictionary dictionary];
119118
_scanningServiceUUIDs = [NSMutableArray array];
120119
_l2capStreamCompletionHandlers = [NSMutableDictionary dictionary];
121120
_l2capPSM = 0;
121+
_centralManager.centralDelegate = self;
122122
}
123123
return self;
124124
}

‎internal/platform/implementation/apple/Tests/GNCFakeNotificationCenter.m‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,17 +45,28 @@ - (instancetype)init {
4545
GNCFakeNotificationObserver *observer = [[GNCFakeNotificationObserver alloc] init];
4646
observer.name = name;
4747
observer.block = block;
48-
[_observers addObject:observer];
48+
@synchronized(self) {
49+
[_observers addObject:observer];
50+
}
4951
return observer;
5052
}
5153

5254
- (void)removeObserver:(id)observer {
53-
[_observers removeObject:observer];
55+
@synchronized(self) {
56+
NSUInteger index = [_observers indexOfObjectIdenticalTo:observer];
57+
if (index != NSNotFound) {
58+
[_observers removeObjectAtIndex:index];
59+
}
60+
}
5461
}
5562

5663
- (void)postNotificationName:(NSNotificationName)aName object:(nullable id)anObject {
5764
NSNotification *notification = [NSNotification notificationWithName:aName object:anObject];
58-
for (GNCFakeNotificationObserver *observer in [_observers copy]) {
65+
NSArray<GNCFakeNotificationObserver *> *observersCopy;
66+
@synchronized(self) {
67+
observersCopy = [_observers copy];
68+
}
69+
for (GNCFakeNotificationObserver *observer in observersCopy) {
5970
if ([observer.name isEqualToString:aName]) {
6071
observer.block(notification);
6172
}

‎internal/platform/implementation/apple/Tests/GNCScheduledExecutorTest.mm‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -82,15 +82,16 @@ - (void)testFailtoScheduleAfterShutdown {
8282
dispatch_queue_t queue = dispatch_get_global_queue(DISPATCH_TARGET_QUEUE_DEFAULT, 0);
8383
XCTestExpectation *expectation = [self expectationWithDescription:@"finished"];
8484

85-
const NSTimeInterval delay = 0.1;
86-
executor->Schedule([self]() { self.counter++; }, absl::Milliseconds(delay));
85+
const int delay_ms = 100;
86+
executor->Schedule([self]() { self.counter++; }, absl::Milliseconds(delay_ms));
8787

88-
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delay * 2 * NSEC_PER_SEC)), queue, ^{
89-
XCTAssertEqual(self.counter, 0);
90-
[expectation fulfill];
91-
});
88+
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delay_ms / 1000.0 * 2 * NSEC_PER_SEC)),
89+
queue, ^{
90+
XCTAssertEqual(self.counter, 0);
91+
[expectation fulfill];
92+
});
9293

93-
[self waitForExpectationsWithTimeout:delay * 5 handler:nil];
94+
[self waitForExpectationsWithTimeout:delay_ms / 1000.0 * 5 handler:nil];
9495
}
9596

9697
// Tests that fails to cancel when the executor is shut down.
@@ -99,7 +100,8 @@ - (void)testFailToCancelAfterShutdown {
99100

100101
executor->Shutdown();
101102

102-
auto cancelable = executor->Schedule([self]() { self.counter++; }, absl::Seconds(0.1));
103+
const int delay_ms = 100;
104+
auto cancelable = executor->Schedule([self]() { self.counter++; }, absl::Milliseconds(delay_ms));
103105

104106
XCTAssert(cancelable.get() == nullptr);
105107
}
@@ -111,17 +113,18 @@ - (void)testShutdownToFailExistingTask {
111113
dispatch_queue_t queue = dispatch_get_global_queue(DISPATCH_TARGET_QUEUE_DEFAULT, 0);
112114
XCTestExpectation *expectation = [self expectationWithDescription:@"finished"];
113115

114-
const NSTimeInterval delay = 0.1;
115-
executor->Schedule([self]() { self.counter++; }, absl::Milliseconds(delay));
116+
const int delay_ms = 100;
117+
executor->Schedule([self]() { self.counter++; }, absl::Milliseconds(delay_ms));
116118

117119
executor->Shutdown();
118120

119-
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delay * 2 * NSEC_PER_SEC)), queue, ^{
120-
XCTAssertEqual(self.counter, 0);
121-
[expectation fulfill];
122-
});
121+
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delay_ms / 1000.0 * 2 * NSEC_PER_SEC)),
122+
queue, ^{
123+
XCTAssertEqual(self.counter, 0);
124+
[expectation fulfill];
125+
});
123126

124-
[self waitForExpectationsWithTimeout:delay * 5 handler:nil];
127+
[self waitForExpectationsWithTimeout:delay_ms / 1000.0 * 5 handler:nil];
125128
}
126129

127130
// Tests that a canceled runnable doesn't run.
@@ -130,15 +133,15 @@ - (void)testCancelable {
130133

131134
// This runnable should run but not increment the counter because it sleeps for longer than the
132135
// cancel below.
133-
const NSTimeInterval delay = 0.1;
134-
auto cancelable = executor->Schedule([self]() { self.counter++; }, absl::Seconds(delay));
136+
const int delay_ms = 100;
137+
auto cancelable = executor->Schedule([self]() { self.counter++; }, absl::Milliseconds(delay_ms));
135138
XCTAssert(cancelable.get() != nullptr);
136139

137140
// Cancel the runnable.
138141
cancelable->Cancel();
139142

140143
// Wait for the time interval to pass and verify that it didn't run.
141-
[NSThread sleepForTimeInterval:delay * 1.1];
144+
[NSThread sleepForTimeInterval:delay_ms / 1000.0 * 1.1];
142145
XCTAssertEqual(self.counter, 0);
143146
}
144147

‎internal/platform/implementation/apple/Tests/GNCTimerTest.mm‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ - (void)testPeriodicTimer {
7575

7676
[self waitForExpectationsWithTimeout:1.0 handler:nil];
7777
XCTAssertTrue(timer->Stop());
78-
XCTAssertEqual(fireCount.load(), 2);
78+
XCTAssertGreaterThanOrEqual(fireCount.load(), 2);
7979
}
8080

8181
- (void)testRestart {

‎internal/platform/implementation/apple/Tests/ble_medium_test.mm‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -646,22 +646,26 @@ - (void)testHandleAdvertisementFound_KnownPeripheral_SameData_WithinThreshold {
646646
NSDictionary<CBUUID *, NSData *> *serviceData =
647647
@{[CBUUID UUIDWithString:kTestServiceUUIDString] : [NSData dataWithBytes:"test" length:4]};
648648

649-
__block XCTestExpectation *expectation1 = [self expectationWithDescription:@"Callback 1"];
649+
// Expect the advertisement found callback to be called exactly once.
650+
XCTestExpectation *expectation1 = [self expectationWithDescription:@"Callback 1"];
650651
XCTestExpectation *expectation2 = [self expectationWithDescription:@"Callback 2"];
651652
expectation2.inverted = YES; // Should NOT be called.
652653

654+
auto callbackCount = std::make_shared<int>(0);
653655
nearby::api::ble::BleMedium::ScanCallback callback = {
654-
.advertisement_found_cb = std::function<void(nearby::api::ble::BlePeripheral::UniqueId,
655-
nearby::api::ble::BleAdvertisementData)>(
656-
^(nearby::api::ble::BlePeripheral::UniqueId peripheral_id,
657-
const nearby::api::ble::BleAdvertisementData &advertisement) {
658-
if ([expectation1.description isEqualToString:@"Callback 1"]) {
656+
.advertisement_found_cb =
657+
[callbackCount, expectation1, expectation2](
658+
nearby::api::ble::BlePeripheral::UniqueId peripheral_id,
659+
const nearby::api::ble::BleAdvertisementData &advertisement) {
660+
(*callbackCount)++;
661+
if (*callbackCount == 1) {
659662
[expectation1 fulfill];
660-
expectation1 = nil; // Prevent double fulfillment
661663
} else {
664+
// Any subsequent call within the threshold fulfills the inverted expectation2, causing
665+
// the test to fail.
662666
[expectation2 fulfill];
663667
}
664-
})};
668+
}};
665669
_medium->StartScanning(nearby::Uuid(0, 0), nearby::api::ble::TxPowerLevel::kUltraLow,
666670
std::move(callback));
667671
if (_fakeGNCBLEMedium.advertisementFoundHandler) {

‎internal/platform/implementation/apple/Tests/ble_server_socket_test.mm‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ - (void)testBleServerSocketAccept {
4545
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
4646
std::unique_ptr<nearby::api::ble::BleSocket> clientSocket = _serverSocket->Accept();
4747
XCTAssertNotEqual(clientSocket.get(), nullptr);
48+
clientSocket.reset();
4849
[expectation fulfill];
4950
});
5051

@@ -60,6 +61,7 @@ - (void)testBleServerSocketClose {
6061
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
6162
std::unique_ptr<nearby::api::ble::BleSocket> clientSocket = _serverSocket->Accept();
6263
XCTAssertEqual(clientSocket.get(), nullptr);
64+
clientSocket.reset();
6365
[expectation fulfill];
6466
});
6567

‎internal/platform/implementation/apple/ble_medium.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -231,11 +231,12 @@ class BleMedium : public api::ble::BleMedium {
231231
GNSPeripheralManager *socketPeripheralManager_;
232232
GNSCentralManager *socketCentralManager_;
233233

234+
absl::Mutex scan_callback_mutex_;
234235
// Used for the blocking version of StartAdvertising and only has an advertisement found callback.
235-
api::ble::BleMedium::ScanCallback scan_cb_;
236+
api::ble::BleMedium::ScanCallback scan_cb_ ABSL_GUARDED_BY(scan_callback_mutex_);
236237
// Used for the async version of StartAdvertising and has both an advertisement found and result
237238
// callback.
238-
api::ble::BleMedium::ScanningCallback scanning_cb_;
239+
api::ble::BleMedium::ScanningCallback scanning_cb_ ABSL_GUARDED_BY(scan_callback_mutex_);
239240

240241
absl::Mutex l2cap_server_socket_mutex_;
241242
BleL2capServerSocket *l2cap_server_socket_ptr_ = nullptr;

‎internal/platform/implementation/apple/ble_medium.mm‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,16 @@
7575
}
7676

7777
// ble_medium.mm
78-
BleMedium::~BleMedium() { [medium_ stop]; }
78+
// StopScanning(), StopAdvertising(), and callback_executor_.Shutdown() are called to ensure proper
79+
// cleanup of resources upon object destruction. This is crucial for preventing leaks, hangs, or
80+
// crashes if active operations are still in progress when the medium is being torn down, further
81+
// improving overall stability and reducing flakiness.
82+
BleMedium::~BleMedium() {
83+
StopScanning();
84+
StopAdvertising();
85+
[medium_ stop];
86+
callback_executor_.Shutdown();
87+
}
7988

8089
std::unique_ptr<api::ble::BleMedium::AdvertisingSession> BleMedium::StartAdvertising(
8190
const api::ble::BleAdvertisementData &advertising_data,
@@ -173,6 +182,7 @@
173182
}
174183
#endif
175184

185+
absl::MutexLock lock(&scan_callback_mutex_);
176186
if (scanning_cb_.advertisement_found_cb) {
177187
scanning_cb_.advertisement_found_cb(unique_id, data);
178188
}
@@ -185,7 +195,10 @@
185195
const Uuid &service_uuid, api::ble::TxPowerLevel tx_power_level,
186196
api::ble::BleMedium::ScanningCallback callback) {
187197
CBUUID *serviceUUID = CBUUID128FromCPP(service_uuid);
188-
scanning_cb_ = std::move(callback);
198+
{
199+
absl::MutexLock lock(&scan_callback_mutex_);
200+
scanning_cb_ = std::move(callback);
201+
}
189202

190203
// Clear the map of discovered peripherals only when we are starting a new scan. If we cleared the
191204
// map every time we stopped a scan, we would not be able to connect to peripherals that we
@@ -207,6 +220,7 @@
207220
}
208221
completionHandler:^(NSError *error) {
209222
blockError = error;
223+
absl::MutexLock lock(&scan_callback_mutex_);
210224
if (scanning_cb_.start_scanning_result) {
211225
scanning_cb_.start_scanning_result(
212226
error == nil ? absl::OkStatus()
@@ -218,6 +232,7 @@
218232
dispatch_time_t timeout = dispatch_time(DISPATCH_TIME_NOW, kApiTimeoutInSeconds * NSEC_PER_SEC);
219233
if (dispatch_semaphore_wait(semaphore, timeout) != 0) {
220234
GNCLoggerError(@"Start scanning operation timed out.");
235+
absl::MutexLock lock(&scan_callback_mutex_);
221236
if (scanning_cb_.start_scanning_result) {
222237
scanning_cb_.start_scanning_result(absl::DeadlineExceededError("Start scanning timed out"));
223238
}
@@ -239,7 +254,10 @@
239254
bool BleMedium::StartScanning(const Uuid &service_uuid, api::ble::TxPowerLevel tx_power_level,
240255
api::ble::BleMedium::ScanCallback callback) {
241256
CBUUID *serviceUUID = CBUUID128FromCPP(service_uuid);
242-
scan_cb_ = std::move(callback);
257+
{
258+
absl::MutexLock lock(&scan_callback_mutex_);
259+
scan_cb_ = std::move(callback);
260+
}
243261

244262
// Clear the map of discovered peripherals only when we are starting a new scan. If we cleared the
245263
// map every time we stopped a scan, we would not be able to connect to peripherals that we
@@ -286,7 +304,10 @@
286304
[serviceUUIDs addObject:CBUUID128FromCPP(service_uuid)];
287305
}
288306

289-
scan_cb_ = std::move(callback);
307+
{
308+
absl::MutexLock lock(&scan_callback_mutex_);
309+
scan_cb_ = std::move(callback);
310+
}
290311

291312
// Clear the map of discovered peripherals only when we are starting a new scan. If we cleared the
292313
// map every time we stopped a scan, we would not be able to connect to peripherals that we

0 commit comments

Comments
 (0)