From 69927d65c3b9f3f65ff34b27140b3d2ad2d1c1ec Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 24 Apr 2015 13:32:48 -0700 Subject: [PATCH] Addressed review comments --- src/objective-c/GRPCClient/GRPCCall.m | 38 ++++++-------- .../GRPCClient/private/GRPCCompletionQueue.m | 4 +- .../GRPCClient/private/GRPCWrappedCall.h | 13 ++--- .../GRPCClient/private/GRPCWrappedCall.m | 51 +++++++++++-------- .../GRPCClient/private/NSData+GRPC.m | 6 +-- 5 files changed, 56 insertions(+), 56 deletions(-) diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 5faccdca4ca..8ae01157b7d 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -41,7 +41,7 @@ #import "private/GRPCCompletionQueue.h" #import "private/GRPCDelegateWrapper.h" #import "private/GRPCMethodName+HTTP2Encoding.h" -#import "GRPCWrappedCall.h" +#import "private/GRPCWrappedCall.h" #import "private/NSData+GRPC.h" #import "private/NSDictionary+GRPC.h" #import "private/NSError+GRPC.h" @@ -176,7 +176,7 @@ static void AssertNoErrorInCall(grpc_call_error error) { // Only called from the call queue. // The handler will be called from the network queue. - (void)startReadWithHandler:(GRPCCompletionHandler)handler { - [_wrappedCall startBatch:@{@(GRPC_OP_RECV_MESSAGE): @YES} handleCompletion:handler]; + [_wrappedCall startBatchWithOperations:@{@(GRPC_OP_RECV_MESSAGE): @YES} handleCompletion:handler]; } // Called initially from the network queue once response headers are received, @@ -191,15 +191,11 @@ static void AssertNoErrorInCall(grpc_call_error error) { } __weak GRPCCall *weakSelf = self; __weak GRPCDelegateWrapper *weakWriteable = _responseWriteable; - + dispatch_async(_callQueue, ^{ - [weakSelf startReadWithHandler:^(NSDictionary *event) { - NSData *data = event[[NSNumber numberWithInt:GRPC_OP_RECV_MESSAGE]]; + [weakSelf startReadWithHandler:^(NSDictionary *result) { + NSData *data = result[@(GRPC_OP_RECV_MESSAGE)]; if (data == [NSNull null]) { - data = nil; - } - if (data == nil) { - // No more responses from the server return; } if (!data) { @@ -226,11 +222,9 @@ static void AssertNoErrorInCall(grpc_call_error error) { #pragma mark Send headers +// TODO(jcanizales): Rename to commitHeaders. - (void)sendHeaders:(NSDictionary *)metadata { - if (metadata == nil) { - metadata = @{}; - } - [_wrappedCall startBatch:@{@(GRPC_OP_SEND_INITIAL_METADATA): metadata} handleCompletion:^(NSDictionary * dict){}]; + [_wrappedCall startBatchWithOperations:@{@(GRPC_OP_SEND_INITIAL_METADATA): metadata?:@{}} handleCompletion:nil]; } #pragma mark GRXWriteable implementation @@ -240,7 +234,7 @@ static void AssertNoErrorInCall(grpc_call_error error) { - (void)writeMessage:(NSData *)message withErrorHandler:(void (^)())errorHandler { __weak GRPCCall *weakSelf = self; - GRPCCompletionHandler resumingHandler = ^(NSDictionary *event) { + GRPCCompletionHandler resumingHandler = ^(NSDictionary *result) { // Resume the request writer (even in the case of error). // TODO(jcanizales): No need to do it in the case of errors anymore? GRPCCall *strongSelf = weakSelf; @@ -248,7 +242,7 @@ static void AssertNoErrorInCall(grpc_call_error error) { strongSelf->_requestWriter.state = GRXWriterStateStarted; } }; - [_wrappedCall startBatch:@{@(GRPC_OP_SEND_MESSAGE): message} handleCompletion:resumingHandler]; + [_wrappedCall startBatchWithOperations:@{@(GRPC_OP_SEND_MESSAGE): message} handleCompletion:resumingHandler errorHandler:errorHandler]; } - (void)didReceiveValue:(id)value { @@ -271,7 +265,7 @@ static void AssertNoErrorInCall(grpc_call_error error) { // Only called from the call queue. The error handler will be called from the // network queue if the requests stream couldn't be closed successfully. - (void)finishRequestWithErrorHandler:(void (^)())errorHandler { - [_wrappedCall startBatch:@{@(GRPC_OP_SEND_CLOSE_FROM_CLIENT): @YES} handleCompletion:^(NSDictionary *event){}]; + [_wrappedCall startBatchWithOperations:@{@(GRPC_OP_SEND_CLOSE_FROM_CLIENT): @YES} handleCompletion:nil errorHandler:errorHandler]; } - (void)didFinishWithError:(NSError *)errorOrNil { @@ -297,22 +291,22 @@ static void AssertNoErrorInCall(grpc_call_error error) { // The second one (completionHandler), whenever the RPC finishes for any reason. - (void)invokeCallWithMetadataHandler:(GRPCCompletionHandler)metadataHandler completionHandler:(GRPCCompletionHandler)completionHandler { - [_wrappedCall startBatch:@{@(GRPC_OP_RECV_INITIAL_METADATA): @YES} handleCompletion:metadataHandler]; - [_wrappedCall startBatch:@{@(GRPC_OP_RECV_STATUS_ON_CLIENT): @YES} handleCompletion:completionHandler]; + [_wrappedCall startBatchWithOperations:@{@(GRPC_OP_RECV_INITIAL_METADATA): @YES} handleCompletion:metadataHandler]; + [_wrappedCall startBatchWithOperations:@{@(GRPC_OP_RECV_STATUS_ON_CLIENT): @YES} handleCompletion:completionHandler]; } - (void)invokeCall { __weak GRPCCall *weakSelf = self; - [self invokeCallWithMetadataHandler:^(NSDictionary *event) { + [self invokeCallWithMetadataHandler:^(NSDictionary *result) { // Response metadata received. GRPCCall *strongSelf = weakSelf; if (strongSelf) { - strongSelf.responseMetadata = event[@(GRPC_OP_RECV_INITIAL_METADATA)]; + strongSelf.responseMetadata = result[@(GRPC_OP_RECV_INITIAL_METADATA)]; [strongSelf startNextRead]; } - } completionHandler:^(NSDictionary *event) { + } completionHandler:^(NSDictionary *result) { // TODO(jcanizales): Merge HTTP2 trailers into response metadata. - id error = event[@(GRPC_OP_RECV_STATUS_ON_CLIENT)]; + id error = result[@(GRPC_OP_RECV_STATUS_ON_CLIENT)]; if (error == [NSNull null]) { error = nil; } diff --git a/src/objective-c/GRPCClient/private/GRPCCompletionQueue.m b/src/objective-c/GRPCClient/private/GRPCCompletionQueue.m index e8ffbc66712..adee053406c 100644 --- a/src/objective-c/GRPCClient/private/GRPCCompletionQueue.m +++ b/src/objective-c/GRPCClient/private/GRPCCompletionQueue.m @@ -46,7 +46,7 @@ - (instancetype)init { if ((self = [super init])) { _unmanagedQueue = grpc_completion_queue_create(); - + // This is for the following block to capture the pointer by value (instead // of retaining self and doing self->_unmanagedQueue). This is essential // because the block doesn't end until after grpc_completion_queue_shutdown @@ -54,7 +54,7 @@ // anymore (i.e. on self dealloc). So the block would never end if it // retained self. grpc_completion_queue *unmanagedQueue = _unmanagedQueue; - + // Start a loop on a concurrent queue to read events from the completion // queue and dispatch each. static dispatch_once_t initialization; diff --git a/src/objective-c/GRPCClient/private/GRPCWrappedCall.h b/src/objective-c/GRPCClient/private/GRPCWrappedCall.h index 2557267fe6f..405ea339793 100644 --- a/src/objective-c/GRPCClient/private/GRPCWrappedCall.h +++ b/src/objective-c/GRPCClient/private/GRPCWrappedCall.h @@ -31,21 +31,18 @@ * */ -#ifndef Pods_GRPCWrappedCall_h -#define Pods_GRPCWrappedCall_h - #import #import "GRPCChannel.h" typedef void(^GRPCCompletionHandler)(NSDictionary *); -@interface GRPCWrappedCall:NSObject; +@interface GRPCWrappedCall : NSObject + +- (instancetype)initWithChannel:(GRPCChannel *)channel method:(NSString *)method host:(NSString *)host NS_DESIGNATED_INITIALIZER; -- (instancetype)initWithChannel:(GRPCChannel *)channel method:(NSString *)method host:(NSString *)host; +- (void)startBatchWithOperations:(NSDictionary *)ops handleCompletion:(GRPCCompletionHandler)handleCompletion errorHandler:(void(^)())errorHandler; -- (void)startBatch:(NSDictionary *)ops handleCompletion:(GRPCCompletionHandler)handleCompletion; +- (void)startBatchWithOperations:(NSDictionary *)ops handleCompletion:(GRPCCompletionHandler)handleCompletion; - (void)cancel; @end - -#endif diff --git a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m index 308d43bb58c..a2acb4b7086 100644 --- a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m +++ b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m @@ -31,19 +31,19 @@ * */ -#import #import "GRPCWrappedCall.h" +#import +#include +#include +#include #import "GRPCCompletionQueue.h" #import "NSDictionary+GRPC.h" #import "NSData+GRPC.h" #import "NSError+GRPC.h" -#include -#include -#include @implementation GRPCWrappedCall{ - grpc_call *call; - GRPCCompletionQueue *queue; + grpc_call *_call; + GRPCCompletionQueue *_queue; } - (instancetype)init { @@ -61,19 +61,21 @@ grpc_init(); }); - const char *method_str = [method UTF8String]; - const char *host_str = [host UTF8String]; - queue = [GRPCCompletionQueue completionQueue]; - call = grpc_channel_create_call(channel.unmanagedChannel, queue.unmanagedQueue, method_str, host_str, gpr_inf_future); - if (call == NULL) { + _queue = [GRPCCompletionQueue completionQueue]; + _call = grpc_channel_create_call(channel.unmanagedChannel, _queue.unmanagedQueue, method.UTF8String, host.UTF8String, gpr_inf_future); + if (_call == NULL) { return nil; } } return self; } -- (void)startBatch:(NSDictionary *)ops handleCompletion:(GRPCCompletionHandler)handleCompletion { - size_t nops = ops.count; +- (void)startBatchWithOperations:(NSDictionary *)operations handleCompletion:(GRPCCompletionHandler)handleCompletion { + [self startBatchWithOperations:operations handleCompletion:handleCompletion errorHandler:nil]; +} + +- (void)startBatchWithOperations:(NSDictionary *)operations handleCompletion:(GRPCCompletionHandler)handleCompletion errorHandler:(void (^)())errorHandler { + size_t nops = operations.count; grpc_op *ops_array = gpr_malloc(nops * sizeof(grpc_op)); size_t index = 0; NSMutableDictionary * __block opProcessors = [NSMutableDictionary new]; @@ -86,12 +88,13 @@ grpc_status_code *status_code; char **status_details; size_t *status_details_capacity; - for (id key in ops) { + for (id key in operations) { id (^opBlock)(void); grpc_op *current = &ops_array[index]; switch ([key intValue]) { case GRPC_OP_SEND_INITIAL_METADATA: - current->data.send_initial_metadata.count = [ops[key] grpc_toMetadataArray:&send_metadata]; + // TODO(jcanizales): Name the type of current->data.send_initial_metadata in the C library so a pointer to it can be returned from methods. + current->data.send_initial_metadata.count = [operations[key] grpc_toMetadataArray:&send_metadata]; current->data.send_initial_metadata.metadata = send_metadata; opBlock = ^{ gpr_free(send_metadata); @@ -99,7 +102,7 @@ }; break; case GRPC_OP_SEND_MESSAGE: - send_message = [ops[key] grpc_byteBuffer]; + send_message = [operations[key] grpc_byteBuffer]; current->data.send_message = send_message; opBlock = ^{ grpc_byte_buffer_destroy(send_message); @@ -162,9 +165,13 @@ current->op = [key intValue]; [opProcessors setObject:opBlock forKey:key]; } - grpc_call_error error = grpc_call_start_batch(call, ops_array, nops, (__bridge_retained void *)(^(grpc_op_error error){ + grpc_call_error error = grpc_call_start_batch(_call, ops_array, nops, (__bridge_retained void *)(^(grpc_op_error error){ if (error != GRPC_OP_OK) { - [NSException raise:@"Operation Exception" format:@"The batch failed with an unknown error"]; + if (errorHandler) { + errorHandler(); + } else { + [NSException raise:@"Operation Exception" format:@"The batch failed with an unknown error"]; + } } NSMutableDictionary *result = [NSMutableDictionary new]; for (id key in opProcessors) { @@ -175,7 +182,9 @@ } [result setObject:value forKey:key]; } - handleCompletion(result); + if (handleCompletion) { + handleCompletion(result); + } })); if (error != GRPC_CALL_OK) { [NSException raise:NSInvalidArgumentException format:@"The batch did not start successfully"]; @@ -183,11 +192,11 @@ } - (void)cancel { - grpc_call_cancel(call); + grpc_call_cancel(_call); } - (void)dealloc { - grpc_call_destroy(call); + grpc_call_destroy(_call); } @end \ No newline at end of file diff --git a/src/objective-c/GRPCClient/private/NSData+GRPC.m b/src/objective-c/GRPCClient/private/NSData+GRPC.m index 951862c56a9..6ea4ce979ef 100644 --- a/src/objective-c/GRPCClient/private/NSData+GRPC.m +++ b/src/objective-c/GRPCClient/private/NSData+GRPC.m @@ -59,9 +59,9 @@ static grpc_byte_buffer *CopyCharArrayToNewByteBuffer(const char *array, size_t @implementation NSData (GRPC) + (instancetype)grpc_dataWithByteBuffer:(grpc_byte_buffer *)buffer { - if (buffer == NULL) { - return nil; - } + if (buffer == NULL) { + return nil; + } NSUInteger length = grpc_byte_buffer_length(buffer); char *array = malloc(length * sizeof(*array)); if (!array) {