Skip to content

Commit b9cfc0c

Browse files
Avoid always creating listening port on the temporary service.
This is a bug introduced by the last temporary service deadlock fix. When temporary service gets a response associated with a EDOObject return value, it calls `[EDOClientService -unwrappedObjectFromObject:]`. Currently this invocation will creating a listening port which is not necessary. To fix this, we move the lazy-load of the listen socket to where it's actually needed, i.e. boxing objects. Other access to the host port won't trigger lazy-load any more. PiperOrigin-RevId: 362571601
1 parent 1c59aa7 commit b9cfc0c

3 files changed

Lines changed: 55 additions & 30 deletions

File tree

Service/Sources/EDOClientService.m

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ + (id)unwrappedObjectFromObject:(id)object {
109109
}
110110
// If there is a service for the current queue, we check if the object belongs to this queue.
111111
// Otherwise, we send EDOObjectAlive message to another service running in the same process.
112-
if ([service.port match:edoObject.servicePort]) {
112+
if ([service isObjectAlive:edoObject]) {
113113
return (__bridge id)(void *)edoObject.remoteAddress;
114114
} else if (edoObject.isLocalEdo) {
115115
// If the underlying object is not a local object (but in the same process) then this could

Service/Sources/EDOHostService.m

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,9 @@ - (EDOObject *)distantObjectForLocalObject:(id)object hostPort:(EDOHostPort *)ho
350350
- (BOOL)isObjectAlive:(EDOObject *)object {
351351
// TODO(haowoo): There can be different strategies to evict the object from the local cache,
352352
// we should check if the object is still in the cache (self.localObjects).
353-
return [self.port match:object.servicePort];
353+
354+
// ivar is used directly here to avoid the service lazily creating listen port.
355+
return [_port match:object.servicePort];
354356
}
355357

356358
- (BOOL)removeObjectWithAddress:(EDOPointerType)remoteAddress {

Service/Tests/UnitTests/EDOServiceTest.m

Lines changed: 51 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@
2222
#import "Service/Sources/EDOClientService.h"
2323
#import "Service/Sources/EDOHostNamingService.h"
2424
#import "Service/Sources/EDOHostService+Private.h"
25+
#import "Service/Sources/EDOObject+Private.h"
26+
#import "Service/Sources/EDOObject.h"
2527
#import "Service/Sources/EDOObjectMessage.h"
2628
#import "Service/Sources/EDORemoteException.h"
2729
#import "Service/Sources/EDOServicePort.h"
@@ -44,9 +46,15 @@
4446

4547
static NSString *const kTestServiceName = @"com.google.edotest.service";
4648

49+
@interface EDOClientService (UnitTest)
50+
51+
+ (id)resolveInstanceFromEDOObject:(EDOObject *)object;
52+
53+
@end
54+
4755
@interface EDOServiceTest : XCTestCase
48-
@property(readonly) id serviceBackgroundMock;
49-
@property(readonly) id serviceMainMock;
56+
@property(readonly) id clientServiceMock;
57+
@property(readonly) NSMutableSet<EDOServicePort *> *objectAliveForwardingPorts;
5058
@property(readonly) EDOHostService *serviceOnBackground;
5159
@property(readonly) EDOHostService *serviceOnMain;
5260
@property(readonly) EDOTestDummy *rootObject;
@@ -68,8 +76,22 @@ - (void)setUp {
6876
_serviceOnMain = [EDOHostService serviceWithRegisteredName:kTestServiceName
6977
rootObject:[[EDOTestDummy alloc] init]
7078
queue:dispatch_get_main_queue()];
71-
_serviceMainMock = OCMPartialMock(_serviceOnMain);
72-
OCMStub([_serviceMainMock isObjectAlive:OCMOCK_ANY]).andReturn(NO);
79+
80+
// Disable the isObjectAlive: check so the object from the differnt queue will not be resolved
81+
// to the underlying object but a remote object
82+
_clientServiceMock = OCMClassMock([EDOClientService class]);
83+
__weak EDOServiceTest *weakSelf = self;
84+
OCMStub([_clientServiceMock
85+
resolveInstanceFromEDOObject:[OCMArg checkWithBlock:^BOOL(EDOObject *obj) {
86+
for (EDOServicePort *servicePort in weakSelf.objectAliveForwardingPorts) {
87+
if ([servicePort match:obj.servicePort]) {
88+
return NO;
89+
}
90+
}
91+
return YES;
92+
}]])
93+
.andReturn(nil);
94+
_objectAliveForwardingPorts = [NSMutableSet set];
7395
[self resetBackgroundService];
7496
}
7597

@@ -81,10 +103,8 @@ - (void)tearDown {
81103
_serviceOnBackground = nil;
82104
_rootObject = nil;
83105

84-
[self.serviceMainMock stopMocking];
85-
[self.serviceBackgroundMock stopMocking];
86-
_serviceMainMock = nil;
87-
_serviceBackgroundMock = nil;
106+
[self.clientServiceMock stopMocking];
107+
_clientServiceMock = nil;
88108

89109
[super tearDown];
90110
}
@@ -168,6 +188,25 @@ - (void)testServiceLifecycleIsBoundToQueue {
168188
[self waitForExpectations:@[ expectServiceNil, expectQueueNil ] timeout:10];
169189
}
170190

191+
- (void)testTemporaryServiceNotCreateChannelIfNoObjectBoxing {
192+
dispatch_queue_t testQueue = dispatch_queue_create("com.google.edotest", DISPATCH_QUEUE_SERIAL);
193+
XCTestExpectation *expectation = [self expectationWithDescription:@"temporary service tested"];
194+
195+
// Temporary service is already used in the main thread, so move the test to the background
196+
// thread.
197+
dispatch_async(testQueue, ^{
198+
EDOHostService *service = [EDOHostService temporaryServiceForCurrentThread];
199+
XCTAssertFalse(service.valid);
200+
201+
XCTAssertNil([EDOHostService serviceForCurrentOriginatingQueue]);
202+
__unused id rootObject = self.rootObjectOnBackground;
203+
XCTAssertFalse(service.valid);
204+
[expectation fulfill];
205+
});
206+
207+
[self waitForExpectations:@[ expectation ] timeout:1.];
208+
}
209+
171210
- (void)testTemporaryServiceLazilyCreatedIfNoServiceAvailable {
172211
dispatch_queue_t testQueue = dispatch_queue_create("com.google.edotest", DISPATCH_QUEUE_SERIAL);
173212
EDOTestDummy *dummyOnBackground = self.rootObjectOnBackground;
@@ -225,12 +264,6 @@ - (void)testTemporaryServiceResolvesLocalObject {
225264
dispatch_queue_t testQueue = dispatch_queue_create("com.google.edotest", DISPATCH_QUEUE_SERIAL);
226265
EDOTestDummy *dummyOnBackground = self.rootObjectOnBackground;
227266

228-
EDOHostService *service = [EDOHostService serviceWithPort:0 rootObject:nil queue:nil];
229-
id serviceMock = OCMPartialMock(service);
230-
OCMStub([[serviceMock classMethod] serviceWithPort:0 rootObject:nil queue:nil])
231-
.andReturn(serviceMock);
232-
OCMStub([serviceMock isObjectAlive:OCMOCK_ANY]).andReturn(NO);
233-
234267
XCTestExpectation *expectation = [self expectationWithDescription:@"nested call completes."];
235268
NSObject *anObject = [[NSObject alloc] init];
236269
__block NSObject *rountTripObject = nil;
@@ -249,7 +282,6 @@ - (void)testTemporaryServiceResolvesLocalObject {
249282
}];
250283
}
251284
[expectation fulfill];
252-
[serviceMock stopMocking];
253285
});
254286
[self waitForExpectations:@[ expectation ] timeout:1.0];
255287
XCTAssertEqual(anObject, rountTripObject);
@@ -388,7 +420,7 @@ - (void)testUnderlyingObjectShouldReturnFromBackgroundQueueInTheSameProcess {
388420
});
389421

390422
// The service on the background queue will resolve to the local address.
391-
[self.serviceBackgroundMock stopMocking];
423+
[self.objectAliveForwardingPorts addObject:self.serviceOnBackground.port];
392424

393425
dispatch_sync(testQueue, ^{
394426
XCTAssertEqual([[self.rootObjectOnBackground returnSelf] class], [EDOTestDummy class],
@@ -411,7 +443,7 @@ - (void)testResolveToUnderlyingInstanceIfInTheSameProcess {
411443
EDOTestDummy *dummyOnBackground = self.rootObjectOnBackground;
412444

413445
// The service on the background queue will resolve to the local address.
414-
[self.serviceBackgroundMock stopMocking];
446+
[self.objectAliveForwardingPorts addObject:self.serviceOnBackground.port];
415447
XCTAssertEqual([[dummyOnBackground returnSelf] class], [EDOTestDummy class]);
416448
XCTAssertEqualObjects([dummyOnBackground returnClassNameWithObject:self], @"EDOObject");
417449
XCTAssertNoThrow(({
@@ -421,7 +453,7 @@ - (void)testResolveToUnderlyingInstanceIfInTheSameProcess {
421453
XCTAssertEqual([dummyOut class], [EDOTestDummy class]);
422454

423455
// The services on both the main queue and the background queue will resolve to the local address.
424-
[self.serviceMainMock stopMocking];
456+
[self.objectAliveForwardingPorts addObject:self.serviceOnMain.port];
425457
XCTAssertEqual([[dummyOnBackground returnSelf] class], [EDOTestDummy class]);
426458
XCTAssertEqualObjects([dummyOnBackground returnClassNameWithObject:self],
427459
NSStringFromClass([self class]));
@@ -432,8 +464,7 @@ - (void)testResolveToUnderlyingInstanceIfInTheSameProcess {
432464
XCTAssertEqual([dummyOut class], [EDOTestDummy class]);
433465

434466
// The service on the main queue will resolve to the local address.
435-
_serviceBackgroundMock = OCMPartialMock(self.serviceOnBackground);
436-
OCMStub([_serviceBackgroundMock isObjectAlive:OCMOCK_ANY]).andReturn(NO);
467+
[self.objectAliveForwardingPorts removeObject:self.serviceOnBackground.port];
437468

438469
XCTAssertEqual([[dummyOnBackground returnSelf] class], NSClassFromString(@"EDOObject"));
439470
XCTAssertEqualObjects([dummyOnBackground returnClassNameWithObject:self],
@@ -628,9 +659,6 @@ - (void)testMockAndNSProxyParameters {
628659
[helperClass invokeMethodsWithProtocol:testProtocol];
629660

630661
OCMVerifyAll(testProtocol);
631-
// Let it resolve otherwise -[isEqual:] causes infinite loop from mocking.
632-
[self.serviceBackgroundMock stopMocking];
633-
[self.serviceMainMock stopMocking];
634662
}
635663

636664
- (void)testEDODescription {
@@ -839,15 +867,10 @@ - (void)fastEnumerateCustom:(EDOTestDummy *)dummy {
839867
}
840868

841869
- (void)resetBackgroundService {
842-
[_serviceBackgroundMock stopMocking];
843870
[_serviceOnBackground invalidate];
844871
_serviceOnBackground = [EDOHostService serviceWithPort:0
845872
rootObject:self.rootObject
846873
queue:self.executionQueue];
847-
// Disable the isObjectAlive: check so the object from the background queue will not be resolved
848-
// to the underlying object but a remote object.
849-
_serviceBackgroundMock = OCMPartialMock(_serviceOnBackground);
850-
OCMStub([_serviceBackgroundMock isObjectAlive:OCMOCK_ANY]).andReturn(NO);
851874
}
852875

853876
@end

0 commit comments

Comments
 (0)