From 2cb1892c77529bfbcf5560ec222a239330ed3aa0 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 28 May 2019 20:59:00 -0700 Subject: [PATCH] address comments --- src/objective-c/GRPCClient/GRPCCall.m | 2 - src/objective-c/GRPCClient/GRPCInterceptor.h | 2 +- src/objective-c/GRPCClient/GRPCInterceptor.m | 4 +- .../InterceptorSample/CacheInterceptor.m | 29 +++-- src/objective-c/tests/InteropTests.m | 110 +++++++++--------- 5 files changed, 73 insertions(+), 74 deletions(-) diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index cee86d6704e..d05f90f7dd2 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -68,8 +68,6 @@ const char *kCFStreamVarName = "grpc_cfstream"; callOptions:(GRPCCallOptions *)callOptions writeDone:(void (^)(void))writeDone; -- (void)receiveNextMessages:(NSUInteger)numberOfMessages; - @end @implementation GRPCRequestOptions diff --git a/src/objective-c/GRPCClient/GRPCInterceptor.h b/src/objective-c/GRPCClient/GRPCInterceptor.h index aa1c4d18565..4b7bcdbff40 100644 --- a/src/objective-c/GRPCClient/GRPCInterceptor.h +++ b/src/objective-c/GRPCClient/GRPCInterceptor.h @@ -210,7 +210,7 @@ NS_ASSUME_NONNULL_BEGIN - (void)forwardPreviousInterceptorWithInitialMetadata:(nullable NSDictionary *)initialMetadata; /** Forward a received message to the previous interceptor in the chain */ -- (void)forwardPreviousIntercetporWithData:(nullable id)data; +- (void)forwardPreviousInterceptorWithData:(nullable id)data; /** Forward call close and trailing metadata to the previous interceptor in the chain */ - (void)forwardPreviousInterceptorCloseWithTrailingMetadata: diff --git a/src/objective-c/GRPCClient/GRPCInterceptor.m b/src/objective-c/GRPCClient/GRPCInterceptor.m index 6be27d74c11..bf043fcd1f5 100644 --- a/src/objective-c/GRPCClient/GRPCInterceptor.m +++ b/src/objective-c/GRPCClient/GRPCInterceptor.m @@ -92,7 +92,7 @@ } /** Forward a received message to the previous interceptor in the chain */ -- (void)forwardPreviousIntercetporWithData:(id)data { +- (void)forwardPreviousInterceptorWithData:(id)data { if ([_previousInterceptor respondsToSelector:@selector(didReceiveData:)]) { id copiedPreviousInterceptor = _previousInterceptor; dispatch_async(copiedPreviousInterceptor.dispatchQueue, ^{ @@ -194,7 +194,7 @@ } - (void)didReceiveData:(id)data { - [_manager forwardPreviousIntercetporWithData:data]; + [_manager forwardPreviousInterceptorWithData:data]; } - (void)didCloseWithTrailingMetadata:(NSDictionary *)trailingMetadata error:(NSError *)error { diff --git a/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.m b/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.m index 4e0cf2c90ed..c8e584d37bc 100644 --- a/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.m +++ b/src/objective-c/examples/InterceptorSample/InterceptorSample/CacheInterceptor.m @@ -167,7 +167,8 @@ dispatch_queue_t _dispatchQueue; BOOL _cacheable; - BOOL _messageSeen; + BOOL _writeMessageSeen; + BOOL _readMessageSeen; GRPCCallOptions *_callOptions; GRPCRequestOptions *_requestOptions; id _requestMessage; @@ -193,7 +194,8 @@ _dispatchQueue = dispatch_queue_create(NULL, DISPATCH_QUEUE_SERIAL); _cacheable = YES; - _messageSeen = NO; + _writeMessageSeen = NO; + _readMessageSeen = NO; _request = nil; _response = nil; } @@ -202,9 +204,7 @@ - (void)startWithRequestOptions:(GRPCRequestOptions *)requestOptions callOptions:(GRPCCallOptions *)callOptions { - NSLog(@"start"); if (requestOptions.safety != GRPCCallSafetyCacheableRequest) { - NSLog(@"no cache"); _cacheable = NO; [_manager startNextInterceptorWithRequest:requestOptions callOptions:callOptions]; } else { @@ -214,21 +214,19 @@ } - (void)writeData:(id)data { - NSLog(@"writeData"); if (!_cacheable) { [_manager writeNextInterceptorWithData:data]; } else { - NSAssert(!_messageSeen, @"CacheInterceptor does not support streaming call"); - if (_messageSeen) { + NSAssert(!_writeMessageSeen, @"CacheInterceptor does not support streaming call"); + if (_writeMessageSeen) { NSLog(@"CacheInterceptor does not support streaming call"); } - _messageSeen = YES; + _writeMessageSeen = YES; _requestMessage = [data copy]; } } - (void)finish { - NSLog(@"finish"); if (!_cacheable) { [_manager finishNextInterceptor]; } else { @@ -236,14 +234,13 @@ _request.path = _requestOptions.path; _request.message = [_requestMessage copy]; _response = [[_context getCachedResponseForRequest:_request] copy]; - NSLog(@"Read cache for %@", _request); if (!_response) { [_manager startNextInterceptorWithRequest:_requestOptions callOptions:_callOptions]; [_manager writeNextInterceptorWithData:_requestMessage]; [_manager finishNextInterceptor]; } else { [_manager forwardPreviousInterceptorWithInitialMetadata:_response.headers]; - [_manager forwardPreviousIntercetporWithData:_response.message]; + [_manager forwardPreviousInterceptorWithData:_response.message]; [_manager forwardPreviousInterceptorCloseWithTrailingMetadata:_response.trailers error:nil]; [_manager shutDown]; } @@ -251,7 +248,6 @@ } - (void)didReceiveInitialMetadata:(NSDictionary *)initialMetadata { - NSLog(@"initialMetadata"); if (_cacheable) { NSDate *deadline = nil; for (NSString *key in initialMetadata) { @@ -286,15 +282,18 @@ } - (void)didReceiveData:(id)data { - NSLog(@"receiveData"); if (_cacheable) { + NSAssert(!_readMessageSeen, @"CacheInterceptor does not support streaming call"); + if (_readMessageSeen) { + NSLog(@"CacheInterceptor does not support streaming call"); + } + _readMessageSeen = YES; _response.message = [data copy]; } - [_manager forwardPreviousIntercetporWithData:data]; + [_manager forwardPreviousInterceptorWithData:data]; } - (void)didCloseWithTrailingMetadata:(NSDictionary *)trailingMetadata error:(NSError *)error { - NSLog(@"close"); if (error == nil && _cacheable) { _response.trailers = [trailingMetadata copy]; [_context setCachedResponse:_response forRequest:_request]; diff --git a/src/objective-c/tests/InteropTests.m b/src/objective-c/tests/InteropTests.m index c96d3436144..6b29d61a173 100644 --- a/src/objective-c/tests/InteropTests.m +++ b/src/objective-c/tests/InteropTests.m @@ -172,19 +172,21 @@ BOOL isRemoteInteropTest(NSString *host) { @interface HookInterceptorFactory : NSObject - (instancetype) - initWithDispatchQueue:(dispatch_queue_t)dispatchQueue - startHook:(void (^)(GRPCRequestOptions *requestOptions, GRPCCallOptions *callOptions, - GRPCInterceptorManager *manager))startHook - writeDataHook:(void (^)(id data, GRPCInterceptorManager *manager))writeDataHook - finishHook:(void (^)(GRPCInterceptorManager *manager))finishHook -receiveNextMessagesHook:(void (^)(NSUInteger numberOfMessages, - GRPCInterceptorManager *manager))receiveNextMessagesHook - responseHeaderHook:(void (^)(NSDictionary *initialMetadata, - GRPCInterceptorManager *manager))responseHeaderHook - responseDataHook:(void (^)(id data, GRPCInterceptorManager *manager))responseDataHook - responseCloseHook:(void (^)(NSDictionary *trailingMetadata, NSError *error, - GRPCInterceptorManager *manager))responseCloseHook - didWriteDataHook:(void (^)(GRPCInterceptorManager *manager))didWriteDataHook; +initWithRequestDispatchQueue:(dispatch_queue_t)requestDispatchQueue + responseDispatchQueue:(dispatch_queue_t)responseDispatchQueue + startHook:(void (^)(GRPCRequestOptions *requestOptions, + GRPCCallOptions *callOptions, + GRPCInterceptorManager *manager))startHook + writeDataHook:(void (^)(id data, GRPCInterceptorManager *manager))writeDataHook + finishHook:(void (^)(GRPCInterceptorManager *manager))finishHook + receiveNextMessagesHook:(void (^)(NSUInteger numberOfMessages, + GRPCInterceptorManager *manager))receiveNextMessagesHook + responseHeaderHook:(void (^)(NSDictionary *initialMetadata, + GRPCInterceptorManager *manager))responseHeaderHook + responseDataHook:(void (^)(id data, GRPCInterceptorManager *manager))responseDataHook + responseCloseHook:(void (^)(NSDictionary *trailingMetadata, NSError *error, + GRPCInterceptorManager *manager))responseCloseHook + didWriteDataHook:(void (^)(GRPCInterceptorManager *manager))didWriteDataHook; - (GRPCInterceptor *)createInterceptorWithManager:(GRPCInterceptorManager *)interceptorManager; @@ -194,7 +196,8 @@ receiveNextMessagesHook:(void (^)(NSUInteger numberOfMessages, - (instancetype) initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager - dispatchQueue:(dispatch_queue_t)dispatchQueue + requestDispatchQueue:(dispatch_queue_t)requestDispatchQueue + responseDispatchQueue:(dispatch_queue_t)responseDispatchQueue startHook:(void (^)(GRPCRequestOptions *requestOptions, GRPCCallOptions *callOptions, GRPCInterceptorManager *manager))startHook @@ -222,25 +225,29 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager void (^_responseCloseHook)(NSDictionary *trailingMetadata, NSError *error, GRPCInterceptorManager *manager); void (^_didWriteDataHook)(GRPCInterceptorManager *manager); - dispatch_queue_t _dispatchQueue; + dispatch_queue_t _requestDispatchQueue; + dispatch_queue_t _responseDispatchQueue; } - (instancetype) - initWithDispatchQueue:(dispatch_queue_t)dispatchQueue - startHook:(void (^)(GRPCRequestOptions *requestOptions, GRPCCallOptions *callOptions, - GRPCInterceptorManager *manager))startHook - writeDataHook:(void (^)(id data, GRPCInterceptorManager *manager))writeDataHook - finishHook:(void (^)(GRPCInterceptorManager *manager))finishHook -receiveNextMessagesHook:(void (^)(NSUInteger numberOfMessages, - GRPCInterceptorManager *manager))receiveNextMessagesHook - responseHeaderHook:(void (^)(NSDictionary *initialMetadata, - GRPCInterceptorManager *manager))responseHeaderHook - responseDataHook:(void (^)(id data, GRPCInterceptorManager *manager))responseDataHook - responseCloseHook:(void (^)(NSDictionary *trailingMetadata, NSError *error, - GRPCInterceptorManager *manager))responseCloseHook - didWriteDataHook:(void (^)(GRPCInterceptorManager *manager))didWriteDataHook { +initWithRequestDispatchQueue:(dispatch_queue_t)requestDispatchQueue + responseDispatchQueue:(dispatch_queue_t)responseDispatchQueue + startHook:(void (^)(GRPCRequestOptions *requestOptions, + GRPCCallOptions *callOptions, + GRPCInterceptorManager *manager))startHook + writeDataHook:(void (^)(id data, GRPCInterceptorManager *manager))writeDataHook + finishHook:(void (^)(GRPCInterceptorManager *manager))finishHook + receiveNextMessagesHook:(void (^)(NSUInteger numberOfMessages, + GRPCInterceptorManager *manager))receiveNextMessagesHook + responseHeaderHook:(void (^)(NSDictionary *initialMetadata, + GRPCInterceptorManager *manager))responseHeaderHook + responseDataHook:(void (^)(id data, GRPCInterceptorManager *manager))responseDataHook + responseCloseHook:(void (^)(NSDictionary *trailingMetadata, NSError *error, + GRPCInterceptorManager *manager))responseCloseHook + didWriteDataHook:(void (^)(GRPCInterceptorManager *manager))didWriteDataHook { if ((self = [super init])) { - _dispatchQueue = dispatchQueue; + _requestDispatchQueue = requestDispatchQueue; + _responseDispatchQueue = responseDispatchQueue; _startHook = startHook; _writeDataHook = writeDataHook; _finishHook = finishHook; @@ -249,14 +256,14 @@ receiveNextMessagesHook:(void (^)(NSUInteger numberOfMessages, _responseDataHook = responseDataHook; _responseCloseHook = responseCloseHook; _didWriteDataHook = didWriteDataHook; - _dispatchQueue = dispatchQueue; } return self; } - (GRPCInterceptor *)createInterceptorWithManager:(GRPCInterceptorManager *)interceptorManager { return [[HookIntercetpor alloc] initWithInterceptorManager:interceptorManager - dispatchQueue:_dispatchQueue + requestDispatchQueue:_requestDispatchQueue + responseDispatchQueue:_responseDispatchQueue startHook:_startHook writeDataHook:_writeDataHook finishHook:_finishHook @@ -281,20 +288,22 @@ receiveNextMessagesHook:(void (^)(NSUInteger numberOfMessages, GRPCInterceptorManager *manager); void (^_didWriteDataHook)(GRPCInterceptorManager *manager); GRPCInterceptorManager *_manager; - dispatch_queue_t _dispatchQueue; + dispatch_queue_t _requestDispatchQueue; + dispatch_queue_t _responseDispatchQueue; } - (dispatch_queue_t)requestDispatchQueue { - return _dispatchQueue; + return _requestDispatchQueue; } - (dispatch_queue_t)dispatchQueue { - return _dispatchQueue; + return _responseDispatchQueue; } - (instancetype) initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager - dispatchQueue:(dispatch_queue_t)dispatchQueue + requestDispatchQueue:(dispatch_queue_t)requestDispatchQueue + responseDispatchQueue:(dispatch_queue_t)responseDispatchQueue startHook:(void (^)(GRPCRequestOptions *requestOptions, GRPCCallOptions *callOptions, GRPCInterceptorManager *manager))startHook @@ -309,8 +318,8 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager GRPCInterceptorManager *manager))responseCloseHook didWriteDataHook:(void (^)(GRPCInterceptorManager *manager))didWriteDataHook { if ((self = [super initWithInterceptorManager:interceptorManager - requestDispatchQueue:dispatchQueue - responseDispatchQueue:dispatchQueue])) { + requestDispatchQueue:requestDispatchQueue + responseDispatchQueue:responseDispatchQueue])) { _startHook = startHook; _writeDataHook = writeDataHook; _finishHook = finishHook; @@ -319,7 +328,8 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager _responseDataHook = responseDataHook; _responseCloseHook = responseCloseHook; _didWriteDataHook = didWriteDataHook; - _dispatchQueue = dispatchQueue; + _requestDispatchQueue = requestDispatchQueue; + _responseDispatchQueue = responseDispatchQueue; _manager = interceptorManager; } return self; @@ -406,7 +416,6 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager } + (void)setUp { - NSLog(@"InteropTest Started, class: %@", [[self class] description]); #ifdef GRPC_COMPILE_WITH_CRONET // Cronet setup [Cronet setHttp2Enabled:YES]; @@ -1316,8 +1325,6 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager } else { [call finish]; } - // DEBUG - NSLog(@"Received message"); } closeCallback:^(NSDictionary *trailingMetadata, NSError *error) { @@ -1328,8 +1335,6 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager @"Received %i responses instead of 4.", index); [expectation fulfill]; - // DEBUG - NSLog(@"Received close"); }] callOptions:options]; [call start]; @@ -1351,7 +1356,8 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager __block NSUInteger responseCloseCount = 0; __block NSUInteger didWriteDataCount = 0; id factory = [[HookInterceptorFactory alloc] - initWithDispatchQueue:dispatch_queue_create(NULL, DISPATCH_QUEUE_SERIAL) + initWithRequestDispatchQueue:dispatch_queue_create(NULL, DISPATCH_QUEUE_SERIAL) + responseDispatchQueue:dispatch_queue_create(NULL, DISPATCH_QUEUE_SERIAL) startHook:^(GRPCRequestOptions *requestOptions, GRPCCallOptions *callOptions, GRPCInterceptorManager *manager) { startCount++; @@ -1371,28 +1377,23 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager } receiveNextMessagesHook:^(NSUInteger numberOfMessages, GRPCInterceptorManager *manager) { receiveNextMessageCount++; - NSLog(@"Interceptor - receive next messages, %lu", (unsigned long)numberOfMessages); [manager receiveNextInterceptorMessages:numberOfMessages]; } responseHeaderHook:^(NSDictionary *initialMetadata, GRPCInterceptorManager *manager) { responseHeaderCount++; - NSLog(@"Interceptor - received initial metadata, %@", initialMetadata); [manager forwardPreviousInterceptorWithInitialMetadata:initialMetadata]; } responseDataHook:^(id data, GRPCInterceptorManager *manager) { responseDataCount++; - NSLog(@"Interceptor - received data, %@", data); - [manager forwardPreviousIntercetporWithData:data]; + [manager forwardPreviousInterceptorWithData:data]; } responseCloseHook:^(NSDictionary *trailingMetadata, NSError *error, GRPCInterceptorManager *manager) { responseCloseCount++; - NSLog(@"Interceptor - received close, %@, %@", trailingMetadata, error); [manager forwardPreviousInterceptorCloseWithTrailingMetadata:trailingMetadata error:error]; } didWriteDataHook:^(GRPCInterceptorManager *manager) { didWriteDataCount++; - NSLog(@"Interceptor - received did-write-data"); [manager forwardPreviousInterceptorDidWriteData]; }]; @@ -1479,7 +1480,8 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager __block NSUInteger responseDataCount = 0; __block NSUInteger responseCloseCount = 0; id factory = [[HookInterceptorFactory alloc] - initWithDispatchQueue:dispatch_queue_create(NULL, DISPATCH_QUEUE_SERIAL) + initWithRequestDispatchQueue:dispatch_queue_create(NULL, DISPATCH_QUEUE_SERIAL) + responseDispatchQueue:dispatch_queue_create(NULL, DISPATCH_QUEUE_SERIAL) startHook:^(GRPCRequestOptions *requestOptions, GRPCCallOptions *callOptions, GRPCInterceptorManager *manager) { startCount++; @@ -1492,11 +1494,11 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager [manager writeNextInterceptorWithData:data]; } else if (index == kCancelAfterWrites) { [manager cancelNextInterceptor]; - [manager forwardPreviousIntercetporWithData:[[RMTStreamingOutputCallResponse + [manager forwardPreviousInterceptorWithData:[[RMTStreamingOutputCallResponse messageWithPayloadSize:responses[index]] data]]; } else { // (index > kCancelAfterWrites) - [manager forwardPreviousIntercetporWithData:[[RMTStreamingOutputCallResponse + [manager forwardPreviousInterceptorWithData:[[RMTStreamingOutputCallResponse messageWithPayloadSize:responses[index]] data]]; } @@ -1514,7 +1516,7 @@ initWithInterceptorManager:(GRPCInterceptorManager *)interceptorManager } responseDataHook:^(id data, GRPCInterceptorManager *manager) { responseDataCount++; - [manager forwardPreviousIntercetporWithData:data]; + [manager forwardPreviousInterceptorWithData:data]; } responseCloseHook:^(NSDictionary *trailingMetadata, NSError *error, GRPCInterceptorManager *manager) {