Skip to content

Commit 36dc9a5

Browse files
edwinwugoogcopybara-github
edwinwugoog
authored and
copybara-github
committed
Fix test flakiness.
PiperOrigin-RevId: 844654834
1 parent 9cf53e9 commit 36dc9a5

File tree

9 files changed

+86
-41
lines changed

9 files changed

+86
-41
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: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ - (void)testFailtoScheduleAfterShutdown {
8383
XCTestExpectation *expectation = [self expectationWithDescription:@"finished"];
8484

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

8888
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(delay * 2 * NSEC_PER_SEC)), queue, ^{
8989
XCTAssertEqual(self.counter, 0);
@@ -112,7 +112,7 @@ - (void)testShutdownToFailExistingTask {
112112
XCTestExpectation *expectation = [self expectationWithDescription:@"finished"];
113113

114114
const NSTimeInterval delay = 0.1;
115-
executor->Schedule([self]() { self.counter++; }, absl::Milliseconds(delay));
115+
executor->Schedule([self]() { self.counter++; }, absl::Seconds(delay));
116116

117117
executor->Shutdown();
118118

‎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: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -646,18 +646,19 @@ - (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+
XCTestExpectation *expectation1 = [self expectationWithDescription:@"Callback 1"];
650650
XCTestExpectation *expectation2 = [self expectationWithDescription:@"Callback 2"];
651651
expectation2.inverted = YES; // Should NOT be called.
652652

653+
__block int callbackCount = 0;
653654
nearby::api::ble::BleMedium::ScanCallback callback = {
654655
.advertisement_found_cb = std::function<void(nearby::api::ble::BlePeripheral::UniqueId,
655656
nearby::api::ble::BleAdvertisementData)>(
656657
^(nearby::api::ble::BlePeripheral::UniqueId peripheral_id,
657658
const nearby::api::ble::BleAdvertisementData &advertisement) {
658-
if ([expectation1.description isEqualToString:@"Callback 1"]) {
659+
callbackCount++;
660+
if (callbackCount == 1) {
659661
[expectation1 fulfill];
660-
expectation1 = nil; // Prevent double fulfillment
661662
} else {
662663
[expectation2 fulfill];
663664
}

‎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: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,12 @@
7575
}
7676

7777
// ble_medium.mm
78-
BleMedium::~BleMedium() { [medium_ stop]; }
78+
BleMedium::~BleMedium() {
79+
StopScanning();
80+
StopAdvertising();
81+
[medium_ stop];
82+
callback_executor_.Shutdown();
83+
}
7984

8085
std::unique_ptr<api::ble::BleMedium::AdvertisingSession> BleMedium::StartAdvertising(
8186
const api::ble::BleAdvertisementData &advertising_data,
@@ -173,6 +178,7 @@
173178
}
174179
#endif
175180

181+
absl::MutexLock lock(&scan_callback_mutex_);
176182
if (scanning_cb_.advertisement_found_cb) {
177183
scanning_cb_.advertisement_found_cb(unique_id, data);
178184
}
@@ -185,7 +191,10 @@
185191
const Uuid &service_uuid, api::ble::TxPowerLevel tx_power_level,
186192
api::ble::BleMedium::ScanningCallback callback) {
187193
CBUUID *serviceUUID = CBUUID128FromCPP(service_uuid);
188-
scanning_cb_ = std::move(callback);
194+
{
195+
absl::MutexLock lock(&scan_callback_mutex_);
196+
scanning_cb_ = std::move(callback);
197+
}
189198

190199
// Clear the map of discovered peripherals only when we are starting a new scan. If we cleared the
191200
// map every time we stopped a scan, we would not be able to connect to peripherals that we
@@ -207,6 +216,7 @@
207216
}
208217
completionHandler:^(NSError *error) {
209218
blockError = error;
219+
absl::MutexLock lock(&scan_callback_mutex_);
210220
if (scanning_cb_.start_scanning_result) {
211221
scanning_cb_.start_scanning_result(
212222
error == nil ? absl::OkStatus()
@@ -218,6 +228,7 @@
218228
dispatch_time_t timeout = dispatch_time(DISPATCH_TIME_NOW, kApiTimeoutInSeconds * NSEC_PER_SEC);
219229
if (dispatch_semaphore_wait(semaphore, timeout) != 0) {
220230
GNCLoggerError(@"Start scanning operation timed out.");
231+
absl::MutexLock lock(&scan_callback_mutex_);
221232
if (scanning_cb_.start_scanning_result) {
222233
scanning_cb_.start_scanning_result(absl::DeadlineExceededError("Start scanning timed out"));
223234
}
@@ -239,7 +250,10 @@
239250
bool BleMedium::StartScanning(const Uuid &service_uuid, api::ble::TxPowerLevel tx_power_level,
240251
api::ble::BleMedium::ScanCallback callback) {
241252
CBUUID *serviceUUID = CBUUID128FromCPP(service_uuid);
242-
scan_cb_ = std::move(callback);
253+
{
254+
absl::MutexLock lock(&scan_callback_mutex_);
255+
scan_cb_ = std::move(callback);
256+
}
243257

244258
// Clear the map of discovered peripherals only when we are starting a new scan. If we cleared the
245259
// map every time we stopped a scan, we would not be able to connect to peripherals that we
@@ -286,7 +300,10 @@
286300
[serviceUUIDs addObject:CBUUID128FromCPP(service_uuid)];
287301
}
288302

289-
scan_cb_ = std::move(callback);
303+
{
304+
absl::MutexLock lock(&scan_callback_mutex_);
305+
scan_cb_ = std::move(callback);
306+
}
290307

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

0 commit comments

Comments
 (0)