From a40ccd858085bfd5148e4e018b62d3f82430ebc6 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Sat, 5 Nov 2016 21:39:44 -0700 Subject: [PATCH 01/26] Packet coalescing Objc layer and interop tests --- .../cronet/transport/cronet_api_dummy.c | 3 +- src/objective-c/GRPCClient/GRPCCall.m | 46 +++++-- .../RxLibrary/GRXImmediateSingleWriter.h | 50 ++++++++ .../RxLibrary/GRXImmediateSingleWriter.m | 83 +++++++++++++ .../RxLibrary/GRXWriter+Immediate.m | 3 +- src/objective-c/tests/InteropTests.m | 64 ++++++++-- .../InteropTestsRemoteWithCronet.m | 11 ++ src/objective-c/tests/Podfile | 1 + .../tests/Tests.xcodeproj/project.pbxproj | 1 + .../objective_c/Cronet/cronet_c_for_grpc.h | 115 ++++++++++++------ 10 files changed, 320 insertions(+), 57 deletions(-) create mode 100644 src/objective-c/RxLibrary/GRXImmediateSingleWriter.h create mode 100644 src/objective-c/RxLibrary/GRXImmediateSingleWriter.m diff --git a/src/core/ext/transport/cronet/transport/cronet_api_dummy.c b/src/core/ext/transport/cronet/transport/cronet_api_dummy.c index 687026c9fde..38755604b97 100644 --- a/src/core/ext/transport/cronet/transport/cronet_api_dummy.c +++ b/src/core/ext/transport/cronet/transport/cronet_api_dummy.c @@ -77,9 +77,8 @@ int cronet_bidirectional_stream_write(cronet_bidirectional_stream* stream, return 0; } -int cronet_bidirectional_stream_cancel(cronet_bidirectional_stream* stream) { +void cronet_bidirectional_stream_cancel(cronet_bidirectional_stream* stream) { GPR_ASSERT(0); - return 0; } #endif /* GRPC_COMPILE_WITH_CRONET */ diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 44393f6b999..0a103223675 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -36,6 +36,7 @@ #include #include #import +#import #import "private/GRPCConnectivityMonitor.h" #import "private/GRPCHost.h" @@ -100,6 +101,10 @@ static NSMutableDictionary *callFlags; GRPCCall *_retainSelf; GRPCRequestHeaders *_requestHeaders; + + BOOL _unaryCall; + + NSMutableArray *_unaryOpBatch; } @synthesize state = _state; @@ -157,6 +162,11 @@ static NSMutableDictionary *callFlags; _requestWriter = requestWriter; _requestHeaders = [[GRPCRequestHeaders alloc] initWithCall:self]; + + if ([requestWriter isKindOfClass:[GRXImmediateSingleWriter class]]) { + _unaryCall = true; + _unaryOpBatch = [[NSMutableArray alloc] init]; + } } return self; } @@ -165,6 +175,9 @@ static NSMutableDictionary *callFlags; - (void)finishWithError:(NSError *)errorOrNil { @synchronized(self) { + if (_state == GRXWriterStateFinished) { + return; + } _state = GRXWriterStateFinished; } @@ -254,9 +267,15 @@ static NSMutableDictionary *callFlags; - (void)sendHeaders:(NSDictionary *)headers { // TODO(jcanizales): Add error handlers for async failures - [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendMetadata alloc] initWithMetadata:headers - flags:[GRPCCall callFlagsForHost:_host path:_path] - handler:nil]]]; + if (!_unaryCall) { + [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendMetadata alloc] initWithMetadata:headers + flags:[GRPCCall callFlagsForHost:_host path:_path] + handler:nil]]]; + } else { + [_unaryOpBatch addObject:[[GRPCOpSendMetadata alloc] initWithMetadata:headers + flags:[GRPCCall callFlagsForHost:_host path:_path] + handler:nil]]; + } } #pragma mark GRXWriteable implementation @@ -275,9 +294,14 @@ static NSMutableDictionary *callFlags; } } }; - [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendMessage alloc] initWithMessage:message - handler:resumingHandler]] - errorHandler:errorHandler]; + if (!_unaryCall) { + [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendMessage alloc] initWithMessage:message + handler:resumingHandler]] + errorHandler:errorHandler]; + } else { + [_unaryOpBatch addObject:[[GRPCOpSendMessage alloc] initWithMessage:message + handler:resumingHandler]]; + } } - (void)writeValue:(id)value { @@ -302,8 +326,14 @@ static NSMutableDictionary *callFlags; // 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 startBatchWithOperations:@[[[GRPCOpSendClose alloc] init]] - errorHandler:errorHandler]; + if (!_unaryOpBatch) { + [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendClose alloc] init]] + errorHandler:errorHandler]; + } else { + [_unaryOpBatch addObject:[[GRPCOpSendClose alloc] init]]; + [_wrappedCall startBatchWithOperations:_unaryOpBatch + errorHandler:errorHandler]; + } } - (void)writesFinishedWithError:(NSError *)errorOrNil { diff --git a/src/objective-c/RxLibrary/GRXImmediateSingleWriter.h b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.h new file mode 100644 index 00000000000..0ec788f756b --- /dev/null +++ b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.h @@ -0,0 +1,50 @@ +/* + * + * Copyright 2016, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#import + +#import "GRXImmediateWriter.h" + +/** + * Utility to construct GRXWriter instances from values that are immediately available when + * required. + */ +@interface GRXImmediateSingleWriter : GRXImmediateWriter + +/** + * Returns a writer that sends the passed value to its writeable and then finishes (releasing the + * value). + */ ++ (GRXWriter *)writerWithValue:(id)value; + +@end diff --git a/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m new file mode 100644 index 00000000000..a0d3b771e8d --- /dev/null +++ b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m @@ -0,0 +1,83 @@ +/* + * + * Copyright 2016, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#import "GRXImmediateSingleWriter.h" + +@implementation GRXImmediateSingleWriter { + id _value; + NSError *_errorOrNil; + id _writeable; +} + +@synthesize state = _state; + +- (instancetype)initWithValue:(id)value error:(NSError *)errorOrNil { + if (self = [super init]) { + _value = value; + _errorOrNil = errorOrNil; + _state = GRXWriterStateNotStarted; + } + return self; +} + ++ (GRXWriter *)writerWithValue:(id)value { + return [[self alloc] initWithValue:value error:nil]; +} + +- (void)startWithWriteable:(id)writeable { + _state = GRXWriterStateStarted; + _writeable = writeable; + [writeable writeValue:_value]; + [self finishWithError:_errorOrNil]; +} + +- (void)finishWithError:(NSError *)errorOrNil { + _state = GRXWriterStateFinished; + _errorOrNil = nil; + _value = nil; + id writeable = _writeable; + _writeable = nil; + [writeable writesFinishedWithError:errorOrNil]; +} + +- (void)setState:(GRXWriterState)newState { + // Manual state transition is not allowed + return; +} + +- (GRXWriter *)map:(id (^)(id))map { + _value = map(_value); + return self; +} + +@end \ No newline at end of file diff --git a/src/objective-c/RxLibrary/GRXWriter+Immediate.m b/src/objective-c/RxLibrary/GRXWriter+Immediate.m index 1d55eb35293..ea6e6814063 100644 --- a/src/objective-c/RxLibrary/GRXWriter+Immediate.m +++ b/src/objective-c/RxLibrary/GRXWriter+Immediate.m @@ -34,6 +34,7 @@ #import "GRXWriter+Immediate.h" #import "GRXImmediateWriter.h" +#import "GRXImmediateSingleWriter.h" @implementation GRXWriter (Immediate) @@ -50,7 +51,7 @@ } + (instancetype)writerWithValue:(id)value { - return [GRXImmediateWriter writerWithValue:value]; + return [GRXImmediateSingleWriter writerWithValue:value]; } + (instancetype)writerWithError:(NSError *)error { diff --git a/src/objective-c/tests/InteropTests.m b/src/objective-c/tests/InteropTests.m index c3935ce1e09..8adf0a6164c 100644 --- a/src/objective-c/tests/InteropTests.m +++ b/src/objective-c/tests/InteropTests.m @@ -45,6 +45,8 @@ #import #import #import +#import +#import #define TEST_TIMEOUT 32 @@ -94,15 +96,6 @@ return 0; } -+ (void)setUp { -#ifdef GRPC_COMPILE_WITH_CRONET - // Cronet setup - [Cronet setHttp2Enabled:YES]; - [Cronet start]; - [GRPCCall useCronetWithEngine:[Cronet getGlobalEngine]]; -#endif -} - - (void)setUp { self.continueAfterFailure = NO; @@ -152,6 +145,59 @@ [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } +// TODO (mxyan): Do the same test for chttp2 +#ifdef GRPC_COMPILE_WITH_CRONET +#ifdef GRPC_CRONET_WITH_PACKET_COALESCING + +static bool coalesced_message_and_eos; + +static void log_processor(gpr_log_func_args *args) { + unsigned long file_len = strlen(args->file); + const char suffix[] = "call.c"; + const int suffix_len = sizeof(suffix) - 1; + const char nops[] = "nops=3"; + + if (file_len > suffix_len && + 0 == strcmp(suffix, &args->file[file_len - suffix_len]) && + strstr(args->message, nops)) { + fprintf(stderr, "%s, %s\n", args->file, args->message); + coalesced_message_and_eos = true; + } +} + +- (void)testPacketCoalescing { + gpr_set_log_verbosity(GPR_LOG_SEVERITY_DEBUG); + grpc_tracer_set_enabled("all", 1); + gpr_set_log_function(log_processor); + coalesced_message_and_eos = false; + + XCTAssertNotNil(self.class.host); + __weak XCTestExpectation *expectation = [self expectationWithDescription:@"LargeUnary"]; + + RMTSimpleRequest *request = [RMTSimpleRequest message]; + request.responseType = RMTPayloadType_Compressable; + request.responseSize = 10; + request.payload.body = [NSMutableData dataWithLength:10]; + + [_service unaryCallWithRequest:request handler:^(RMTSimpleResponse *response, NSError *error) { + XCTAssertNil(error, @"Finished with unexpected error: %@", error); + + RMTSimpleResponse *expectedResponse = [RMTSimpleResponse message]; + expectedResponse.payload.type = RMTPayloadType_Compressable; + expectedResponse.payload.body = [NSMutableData dataWithLength:10]; + XCTAssertEqualObjects(response, expectedResponse); + + XCTAssert(coalesced_message_and_eos); + + [expectation fulfill]; + }]; + + [self waitForExpectationsWithTimeout:16 handler:nil]; +} + +#endif +#endif + - (void)test4MBResponsesAreAccepted { XCTAssertNotNil(self.class.host); __weak XCTestExpectation *expectation = [self expectationWithDescription:@"MaxResponseSize"]; diff --git a/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m b/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m index fab8ad8d25f..793b71a9bee 100644 --- a/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m +++ b/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m @@ -33,6 +33,9 @@ #import +#import +#import + #import "InteropTests.h" static NSString * const kRemoteSSLHost = @"grpc-test.sandbox.googleapis.com"; @@ -43,6 +46,14 @@ static NSString * const kRemoteSSLHost = @"grpc-test.sandbox.googleapis.com"; @implementation InteropTestsRemoteWithCronet ++ (void)setUp { + // Cronet setup + [Cronet setHttp2Enabled:YES]; + [Cronet start]; + [GRPCCall useCronetWithEngine:[Cronet getGlobalEngine]]; + [Cronet startNetLogToFile:@"Documents/cronet_netlog.json" logBytes:YES]; +} + + (NSString *)host { return kRemoteSSLHost; } diff --git a/src/objective-c/tests/Podfile b/src/objective-c/tests/Podfile index 5785b976f2d..d1ef0886fe4 100644 --- a/src/objective-c/tests/Podfile +++ b/src/objective-c/tests/Podfile @@ -92,6 +92,7 @@ post_install do |installer| # GPR_UNREACHABLE_CODE causes "Control may reach end of non-void # function" warning config.build_settings['GCC_WARN_ABOUT_RETURN_TYPE'] = 'NO' + config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_CRONET_WITH_PACKET_COALESCING=1' end end diff --git a/src/objective-c/tests/Tests.xcodeproj/project.pbxproj b/src/objective-c/tests/Tests.xcodeproj/project.pbxproj index c4a6567ae0e..8455e71b026 100644 --- a/src/objective-c/tests/Tests.xcodeproj/project.pbxproj +++ b/src/objective-c/tests/Tests.xcodeproj/project.pbxproj @@ -1296,6 +1296,7 @@ "$(inherited)", "GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS=1", "GRPC_COMPILE_WITH_CRONET=1", + "GRPC_CRONET_WITH_PACKET_COALESCING=1", ); INFOPLIST_FILE = InteropTestsRemoteWithCronet/Info.plist; IPHONEOS_DEPLOYMENT_TARGET = 9.3; diff --git a/third_party/objective_c/Cronet/cronet_c_for_grpc.h b/third_party/objective_c/Cronet/cronet_c_for_grpc.h index 15a511aebd0..3d58a8370ef 100644 --- a/third_party/objective_c/Cronet/cronet_c_for_grpc.h +++ b/third_party/objective_c/Cronet/cronet_c_for_grpc.h @@ -5,6 +5,8 @@ #ifndef COMPONENTS_CRONET_IOS_CRONET_C_FOR_GRPC_H_ #define COMPONENTS_CRONET_IOS_CRONET_C_FOR_GRPC_H_ +#define CRONET_EXPORT __attribute__((visibility("default"))) + #ifdef __cplusplus extern "C" { #endif @@ -15,12 +17,10 @@ extern "C" { /* Opaque object representing Cronet Engine. Created and configured outside * of this API to facilitate sharing with other components */ -typedef struct cronet_engine { void* obj; } cronet_engine; - -void cronet_engine_add_quic_hint(cronet_engine* engine, - const char* host, - int port, - int alternate_port); +typedef struct cronet_engine { + void* obj; + void* annotation; +} cronet_engine; /* Cronet Bidirectional Stream API */ @@ -45,11 +45,12 @@ typedef struct cronet_bidirectional_stream_header_array { /* Set of callbacks used to receive callbacks from bidirectional stream. */ typedef struct cronet_bidirectional_stream_callback { - /* Invoked when request headers are sent. Indicates that stream has initiated - * the request. Consumer may call cronet_bidirectional_stream_write() to start - * writing data. + /* Invoked when the stream is ready for reading and writing. + * Consumer may call cronet_bidirectional_stream_read() to start reading data. + * Consumer may call cronet_bidirectional_stream_write() to start writing + * data. */ - void (*on_request_headers_sent)(cronet_bidirectional_stream* stream); + void (*on_stream_ready)(cronet_bidirectional_stream* stream); /* Invoked when initial response headers are received. * Consumer must call cronet_bidirectional_stream_read() to start reading. @@ -67,20 +68,19 @@ typedef struct cronet_bidirectional_stream_callback { * It may be invoked after on_response_trailers_received()}, if there was * pending read data before trailers were received. * - * If count is 0, it means the remote side has signaled that it will send no - * more data; future calls to cronet_bidirectional_stream_read() will result - * in the on_data_read() callback or on_succeded() callback if + * If |bytes_read| is 0, it means the remote side has signaled that it will + * send no more data; future calls to cronet_bidirectional_stream_read() + * will result in the on_data_read() callback or on_succeded() callback if * cronet_bidirectional_stream_write() was invoked with end_of_stream set to * true. */ void (*on_read_completed)(cronet_bidirectional_stream* stream, char* data, - int count); + int bytes_read); /** * Invoked when all data passed to cronet_bidirectional_stream_write() is - * sent. - * To continue writing, call cronet_bidirectional_stream_write(). + * sent. To continue writing, call cronet_bidirectional_stream_write(). */ void (*on_write_completed)(cronet_bidirectional_stream* stream, const char* data); @@ -117,7 +117,7 @@ typedef struct cronet_bidirectional_stream_callback { void (*on_canceled)(cronet_bidirectional_stream* stream); } cronet_bidirectional_stream_callback; -/* Create a new stream object that uses |engine| and |callback|. All stream +/* Creates a new stream object that uses |engine| and |callback|. All stream * tasks are performed asynchronously on the |engine| network thread. |callback| * methods are invoked synchronously on the |engine| network thread, but must * not run tasks on the current thread to prevent blocking networking operations @@ -129,6 +129,7 @@ typedef struct cronet_bidirectional_stream_callback { * * Both |calback| and |engine| must remain valid until stream is destroyed. */ +CRONET_EXPORT cronet_bidirectional_stream* cronet_bidirectional_stream_create( cronet_engine* engine, void* annotation, @@ -136,15 +137,40 @@ cronet_bidirectional_stream* cronet_bidirectional_stream_create( /* TBD: The following methods return int. Should it be a custom type? */ -/* Destroy stream object. Destroy could be called from any thread, including +/* Destroys stream object. Destroy could be called from any thread, including * network thread, but is posted, so |stream| is valid until calling task is * complete. */ +CRONET_EXPORT int cronet_bidirectional_stream_destroy(cronet_bidirectional_stream* stream); -/* Start the stream by sending request to |url| using |method| and |headers|. If - * |end_of_stream| is true, then no data is expected to be written. +/** + * Disables or enables auto flush. By default, data is flushed after + * every cronet_bidirectional_stream_write(). If the auto flush is disabled, + * the client should explicitly call cronet_bidirectional_stream_flush to flush + * the data. */ +CRONET_EXPORT void cronet_bidirectional_stream_disable_auto_flush( + cronet_bidirectional_stream* stream, + bool disable_auto_flush); + +/** + * Delays sending request headers until cronet_bidirectional_stream_flush() + * is called. This flag is currently only respected when QUIC is negotiated. + * When true, QUIC will send request header frame along with data frame(s) + * as a single packet when possible. + */ +CRONET_EXPORT +void cronet_bidirectional_stream_delay_request_headers_until_flush( + cronet_bidirectional_stream* stream, + bool delay_headers_until_flush); + +/* Starts the stream by sending request to |url| using |method| and |headers|. + * If |end_of_stream| is true, then no data is expected to be written. The + * |method| is HTTP verb, with PUT having a special meaning to mark idempotent + * request, which could use QUIC 0-RTT. + */ +CRONET_EXPORT int cronet_bidirectional_stream_start( cronet_bidirectional_stream* stream, const char* url, @@ -153,46 +179,61 @@ int cronet_bidirectional_stream_start( const cronet_bidirectional_stream_header_array* headers, bool end_of_stream); -/* Read response data into |buffer| of |capacity| length. Must only be called at - * most once in response to each invocation of the - * on_response_headers_received() and on_read_completed() methods of the - * cronet_bidirectional_stream_callback. - * Each call will result in an invocation of one of the callback's - * on_read_completed method if data is read, its on_succeeded() method if - * the stream is closed, or its on_failed() method if there's an error. +/* Reads response data into |buffer| of |capacity| length. Must only be called + * at most once in response to each invocation of the + * on_stream_ready()/on_response_headers_received() and on_read_completed() + * methods of the cronet_bidirectional_stream_callback. + * Each call will result in an invocation of the callback's + * on_read_completed() method if data is read, or its on_failed() method if + * there's an error. The callback's on_succeeded() method is also invoked if + * there is no more data to read and |end_of_stream| was previously sent. */ +CRONET_EXPORT int cronet_bidirectional_stream_read(cronet_bidirectional_stream* stream, char* buffer, int capacity); -/* Read response data into |buffer| of |capacity| length. Must only be called at - * most once in response to each invocation of the - * on_response_headers_received() and on_read_completed() methods of the - * cronet_bidirectional_stream_callback. - * Each call will result in an invocation of one of the callback's - * on_read_completed method if data is read, its on_succeeded() method if - * the stream is closed, or its on_failed() method if there's an error. +/* Writes request data from |buffer| of |buffer_length| length. If auto flush is + * disabled, data will be sent only after cronet_bidirectional_stream_flush() is + * called. + * Each call will result in an invocation the callback's on_write_completed() + * method if data is sent, or its on_failed() method if there's an error. + * The callback's on_succeeded() method is also invoked if |end_of_stream| is + * set and all response data has been read. */ +CRONET_EXPORT int cronet_bidirectional_stream_write(cronet_bidirectional_stream* stream, const char* buffer, - int count, + int buffer_length, bool end_of_stream); +/** + * Flushes pending writes. This method should not be called before invocation of + * on_stream_ready() method of the cronet_bidirectional_stream_callback. + * For each previously called cronet_bidirectional_stream_write() + * a corresponding on_write_completed() callback will be invoked when the buffer + * is sent. + */ +CRONET_EXPORT +void cronet_bidirectional_stream_flush(cronet_bidirectional_stream* stream); + /* Cancels the stream. Can be called at any time after * cronet_bidirectional_stream_start(). The on_canceled() method of * cronet_bidirectional_stream_callback will be invoked when cancelation * is complete and no further callback methods will be invoked. If the * stream has completed or has not started, calling * cronet_bidirectional_stream_cancel() has no effect and on_canceled() will not - * be invoked. At most one callback method may be invoked after + * be invoked. At most one callback method may be invoked after * cronet_bidirectional_stream_cancel() has completed. */ -int cronet_bidirectional_stream_cancel(cronet_bidirectional_stream* stream); +CRONET_EXPORT +void cronet_bidirectional_stream_cancel(cronet_bidirectional_stream* stream); /* Returns true if the |stream| was successfully started and is now done * (succeeded, canceled, or failed). * Returns false if the |stream| stream is not yet started or is in progress. */ +CRONET_EXPORT bool cronet_bidirectional_stream_is_done(cronet_bidirectional_stream* stream); #ifdef __cplusplus From bf803b95638753230eb57bd4f3178b7d49ff2018 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Wed, 8 Feb 2017 11:31:26 -0800 Subject: [PATCH 02/26] Nit fixes --- src/objective-c/GRPCClient/GRPCCall.m | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 0a103223675..1a8fc2e2ef6 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -164,8 +164,8 @@ static NSMutableDictionary *callFlags; _requestHeaders = [[GRPCRequestHeaders alloc] initWithCall:self]; if ([requestWriter isKindOfClass:[GRXImmediateSingleWriter class]]) { - _unaryCall = true; - _unaryOpBatch = [[NSMutableArray alloc] init]; + _unaryCall = YES; + _unaryOpBatch = [NSMutableArray arrayWithCapacity:6]; } } return self; @@ -267,14 +267,13 @@ static NSMutableDictionary *callFlags; - (void)sendHeaders:(NSDictionary *)headers { // TODO(jcanizales): Add error handlers for async failures + GRPCOpSendMetadata *op = [[GRPCOpSendMetadata alloc] initWithMetadata:headers + flags:[GRPCCall callFlagsForHost:_host path:_path] + handler:nil]; if (!_unaryCall) { - [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendMetadata alloc] initWithMetadata:headers - flags:[GRPCCall callFlagsForHost:_host path:_path] - handler:nil]]]; + [_wrappedCall startBatchWithOperations:@[op]]; } else { - [_unaryOpBatch addObject:[[GRPCOpSendMetadata alloc] initWithMetadata:headers - flags:[GRPCCall callFlagsForHost:_host path:_path] - handler:nil]]; + [_unaryOpBatch addObject:op]; } } @@ -294,13 +293,14 @@ static NSMutableDictionary *callFlags; } } }; + + GRPCOpSendMessage *op = [[GRPCOpSendMessage alloc] initWithMessage:message + handler:resumingHandler]; if (!_unaryCall) { - [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendMessage alloc] initWithMessage:message - handler:resumingHandler]] + [_wrappedCall startBatchWithOperations:@[op] errorHandler:errorHandler]; } else { - [_unaryOpBatch addObject:[[GRPCOpSendMessage alloc] initWithMessage:message - handler:resumingHandler]]; + [_unaryOpBatch addObject:op]; } } From 40d7c627bde4c08d40568f62e6f284a6a615fb71 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Wed, 8 Feb 2017 14:41:45 -0800 Subject: [PATCH 03/26] Only test the ObjC layer --- src/objective-c/GRPCClient/GRPCCall+Tests.h | 15 +++++++ src/objective-c/GRPCClient/GRPCCall+Tests.m | 37 ++++++++++++++++ .../GRPCClient/private/GRPCWrappedCall.m | 4 ++ src/objective-c/tests/InteropTests.m | 43 ++++++------------- 4 files changed, 68 insertions(+), 31 deletions(-) diff --git a/src/objective-c/GRPCClient/GRPCCall+Tests.h b/src/objective-c/GRPCClient/GRPCCall+Tests.h index 184ad09c5c8..f1618afdbf4 100644 --- a/src/objective-c/GRPCClient/GRPCCall+Tests.h +++ b/src/objective-c/GRPCClient/GRPCCall+Tests.h @@ -63,4 +63,19 @@ * cache. */ + (void)resetHostSettings; + +/** + * Enables logging of op batches. Memory consumption increases as more ops are logged. + */ ++ (void)enableOpBatchLog:(BOOL)enabled; + +/** + * Add an op batch to log. + */ ++ (void)addOpBatchToLog:(NSArray *)batch; + +/** + * Obtain the logged op batches. Invoking this method will clean the log. + */ ++ (NSArray *)obtainAndCleanOpBatchLog; @end diff --git a/src/objective-c/GRPCClient/GRPCCall+Tests.m b/src/objective-c/GRPCClient/GRPCCall+Tests.m index 656cba8fec6..fe864707e0a 100644 --- a/src/objective-c/GRPCClient/GRPCCall+Tests.m +++ b/src/objective-c/GRPCClient/GRPCCall+Tests.m @@ -64,4 +64,41 @@ + (void)resetHostSettings { [GRPCHost resetAllHostSettings]; } + +static NSMutableArray *opBatchLog = nil; + ++ (void)enableOpBatchLog:(BOOL)enabled { + @synchronized (opBatchLog) { + if (enabled) { + if (!opBatchLog) { + opBatchLog = [NSMutableArray array]; + } + } else { + if (opBatchLog) { + opBatchLog = nil; + } + } + } +} + ++ (void)addOpBatchToLog:(NSArray *)batch { + @synchronized (opBatchLog) { + if (opBatchLog) { + [opBatchLog addObject:batch]; + } + } +} + ++ (NSArray *)obtainAndCleanOpBatchLog { + @synchronized (opBatchLog) { + if (opBatchLog) { + NSArray *out = opBatchLog; + opBatchLog = [NSMutableArray array]; + return out; + } else { + return nil; + } + } +} + @end diff --git a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m index 38fcae0299d..fd624a716ff 100644 --- a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m +++ b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m @@ -44,6 +44,8 @@ #import "NSData+GRPC.h" #import "NSError+GRPC.h" +#import "GRPCCall+Tests.h" + @implementation GRPCOperation { @protected // Most operation subclasses don't set any flags in the grpc_op, and rely on the flag member being @@ -271,6 +273,8 @@ } - (void)startBatchWithOperations:(NSArray *)operations errorHandler:(void (^)())errorHandler { + [GRPCCall addOpBatchToLog:operations]; + size_t nops = operations.count; grpc_op *ops_array = gpr_malloc(nops * sizeof(grpc_op)); size_t i = 0; diff --git a/src/objective-c/tests/InteropTests.m b/src/objective-c/tests/InteropTests.m index 8adf0a6164c..2bd54e17142 100644 --- a/src/objective-c/tests/InteropTests.m +++ b/src/objective-c/tests/InteropTests.m @@ -145,32 +145,7 @@ [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } -// TODO (mxyan): Do the same test for chttp2 -#ifdef GRPC_COMPILE_WITH_CRONET -#ifdef GRPC_CRONET_WITH_PACKET_COALESCING - -static bool coalesced_message_and_eos; - -static void log_processor(gpr_log_func_args *args) { - unsigned long file_len = strlen(args->file); - const char suffix[] = "call.c"; - const int suffix_len = sizeof(suffix) - 1; - const char nops[] = "nops=3"; - - if (file_len > suffix_len && - 0 == strcmp(suffix, &args->file[file_len - suffix_len]) && - strstr(args->message, nops)) { - fprintf(stderr, "%s, %s\n", args->file, args->message); - coalesced_message_and_eos = true; - } -} - - (void)testPacketCoalescing { - gpr_set_log_verbosity(GPR_LOG_SEVERITY_DEBUG); - grpc_tracer_set_enabled("all", 1); - gpr_set_log_function(log_processor); - coalesced_message_and_eos = false; - XCTAssertNotNil(self.class.host); __weak XCTestExpectation *expectation = [self expectationWithDescription:@"LargeUnary"]; @@ -179,6 +154,7 @@ static void log_processor(gpr_log_func_args *args) { request.responseSize = 10; request.payload.body = [NSMutableData dataWithLength:10]; + [GRPCCall enableOpBatchLog:YES]; [_service unaryCallWithRequest:request handler:^(RMTSimpleResponse *response, NSError *error) { XCTAssertNil(error, @"Finished with unexpected error: %@", error); @@ -187,17 +163,22 @@ static void log_processor(gpr_log_func_args *args) { expectedResponse.payload.body = [NSMutableData dataWithLength:10]; XCTAssertEqualObjects(response, expectedResponse); - XCTAssert(coalesced_message_and_eos); - - [expectation fulfill]; + NSArray *opBatches = [GRPCCall obtainAndCleanOpBatchLog]; + for (NSObject *o in opBatches) { + if ([o isKindOfClass:[NSArray class]]) { + NSArray *batch = (NSArray *)o; + if ([batch count] == 3) { + [expectation fulfill]; + break; + } + } + } }]; [self waitForExpectationsWithTimeout:16 handler:nil]; + [GRPCCall enableOpBatchLog:NO]; } -#endif -#endif - - (void)test4MBResponsesAreAccepted { XCTAssertNotNil(self.class.host); __weak XCTestExpectation *expectation = [self expectationWithDescription:@"MaxResponseSize"]; From 5bd16b70133ba3901ed39eb867c34522fdf5778f Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Thu, 9 Feb 2017 16:54:30 -0800 Subject: [PATCH 04/26] Warnings and macro guards for op batch log --- .../{ => internal_testing}/GRPCCall+Tests.h | 13 +- .../{ => internal_testing}/GRPCCall+Tests.m | 48 ++-- .../GRPCClient/private/GRPCOpBatchLog.h | 54 +++++ .../GRPCClient/private/GRPCOpBatchLog.m | 74 ++++++ .../GRPCClient/private/GRPCWrappedCall.m | 6 +- src/objective-c/tests/GRPCClientTests.m | 2 +- src/objective-c/tests/InteropTests.m | 2 +- .../tests/InteropTestsLocalCleartext.m | 2 +- src/objective-c/tests/InteropTestsLocalSSL.m | 2 +- src/objective-c/tests/InteropTestsRemote.m | 2 +- .../InteropTestsRemoteWithCronet.m | 2 +- src/objective-c/tests/Podfile | 6 +- .../tests/Tests.xcodeproj/project.pbxproj | 211 ++++++++++++++++++ .../xcshareddata/xcschemes/AllTests.xcscheme | 6 +- .../InteropTestsLocalCleartext.xcscheme | 6 +- .../xcschemes/InteropTestsLocalSSL.xcscheme | 6 +- .../xcschemes/InteropTestsRemote.xcscheme | 6 +- 17 files changed, 390 insertions(+), 58 deletions(-) rename src/objective-c/GRPCClient/{ => internal_testing}/GRPCCall+Tests.h (88%) rename src/objective-c/GRPCClient/{ => internal_testing}/GRPCCall+Tests.m (80%) create mode 100644 src/objective-c/GRPCClient/private/GRPCOpBatchLog.h create mode 100644 src/objective-c/GRPCClient/private/GRPCOpBatchLog.m diff --git a/src/objective-c/GRPCClient/GRPCCall+Tests.h b/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h similarity index 88% rename from src/objective-c/GRPCClient/GRPCCall+Tests.h rename to src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h index f1618afdbf4..0325f5a192f 100644 --- a/src/objective-c/GRPCClient/GRPCCall+Tests.h +++ b/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h @@ -31,7 +31,7 @@ * */ -#import "GRPCCall.h" +#import "../GRPCCall.h" /** * Methods to let tune down the security of gRPC connections for specific hosts. These shouldn't be @@ -66,16 +66,17 @@ /** * Enables logging of op batches. Memory consumption increases as more ops are logged. + * + * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. + * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. */ + (void)enableOpBatchLog:(BOOL)enabled; -/** - * Add an op batch to log. - */ -+ (void)addOpBatchToLog:(NSArray *)batch; - /** * Obtain the logged op batches. Invoking this method will clean the log. + * + * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. + * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. */ + (NSArray *)obtainAndCleanOpBatchLog; @end diff --git a/src/objective-c/GRPCClient/GRPCCall+Tests.m b/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m similarity index 80% rename from src/objective-c/GRPCClient/GRPCCall+Tests.m rename to src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m index fe864707e0a..5109480bef8 100644 --- a/src/objective-c/GRPCClient/GRPCCall+Tests.m +++ b/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m @@ -33,7 +33,8 @@ #import "GRPCCall+Tests.h" -#import "private/GRPCHost.h" +#import "../private/GRPCHost.h" +#import "../private/GRPCOpBatchLog.h" @implementation GRPCCall (Tests) @@ -65,40 +66,25 @@ [GRPCHost resetAllHostSettings]; } -static NSMutableArray *opBatchLog = nil; - + (void)enableOpBatchLog:(BOOL)enabled { - @synchronized (opBatchLog) { - if (enabled) { - if (!opBatchLog) { - opBatchLog = [NSMutableArray array]; - } - } else { - if (opBatchLog) { - opBatchLog = nil; - } - } - } -} - -+ (void)addOpBatchToLog:(NSArray *)batch { - @synchronized (opBatchLog) { - if (opBatchLog) { - [opBatchLog addObject:batch]; - } - } +#ifdef GRPC_TEST_OBJC + [GRPCOpBatchLog enableOpBatchLog:enabled]; +#else + NSLog(@"This function is for internal testing of gRPC only. " + "It is not part of gRPC's public interface. Do not use in production. " + "To enable, set the preprocessor flag GRPC_TEST_OBJC."); +#endif } + (NSArray *)obtainAndCleanOpBatchLog { - @synchronized (opBatchLog) { - if (opBatchLog) { - NSArray *out = opBatchLog; - opBatchLog = [NSMutableArray array]; - return out; - } else { - return nil; - } - } +#ifdef GRPC_TEST_OBJC + return [GRPCOpBatchLog obtainAndCleanOpBatchLog]; +#else + NSLog(@"This function is for internal testing of gRPC only. " + "It is not part of gRPC's public interface. Do not use in production. " + "To enable, set the preprocessor flag GRPC_TEST_OBJC."); + return nil; +#endif } @end diff --git a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.h b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.h new file mode 100644 index 00000000000..dbc3417acf7 --- /dev/null +++ b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.h @@ -0,0 +1,54 @@ +/* + * + * Copyright 2016, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +/** + * Logs the op batches of a client. Used for testing. + */ +@interface GRPCOpBatchLog : NSObject + +/** + * Enables logging of op batches. Memory consumption increases as more ops are logged. + */ ++ (void)enableOpBatchLog:(BOOL)enabled; + +/** + * Add an op batch to log. + */ ++ (void)addOpBatchToLog:(NSArray *)batch; + +/** + * Obtain the logged op batches. Invoking this method will clean the log. + */ ++ (NSArray *)obtainAndCleanOpBatchLog; + +@end diff --git a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m new file mode 100644 index 00000000000..e5e7100ddf0 --- /dev/null +++ b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m @@ -0,0 +1,74 @@ +/* + * + * Copyright 2016, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#import "GRPCOpBatchLog.h" + +@implementation GRPCOpBatchLog + +NSMutableArray *opBatchLog = nil; + ++ (void)enableOpBatchLog:(BOOL)enabled { + @synchronized (opBatchLog) { + if (enabled) { + if (!opBatchLog) { + opBatchLog = [NSMutableArray array]; + } + } else { + if (opBatchLog) { + opBatchLog = nil; + } + } + } +} + ++ (void)addOpBatchToLog:(NSArray *)batch { + @synchronized (opBatchLog) { + if (opBatchLog) { + [opBatchLog addObject:batch]; + } + } +} + ++ (NSArray *)obtainAndCleanOpBatchLog { + @synchronized (opBatchLog) { + if (opBatchLog) { + NSArray *out = opBatchLog; + opBatchLog = [NSMutableArray array]; + return out; + } else { + return nil; + } + } +} + +@end \ No newline at end of file diff --git a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m index 9d493e1e8a0..4476a41bd97 100644 --- a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m +++ b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m @@ -44,7 +44,7 @@ #import "NSData+GRPC.h" #import "NSError+GRPC.h" -#import "GRPCCall+Tests.h" +#import "GRPCOpBatchLog.h" @implementation GRPCOperation { @protected @@ -276,7 +276,9 @@ } - (void)startBatchWithOperations:(NSArray *)operations errorHandler:(void (^)())errorHandler { - [GRPCCall addOpBatchToLog:operations]; +#ifdef GRPC_TEST_OBJC + [GRPCOpBatchLog addOpBatchToLog:operations]; +#endif size_t nops = operations.count; grpc_op *ops_array = gpr_malloc(nops * sizeof(grpc_op)); diff --git a/src/objective-c/tests/GRPCClientTests.m b/src/objective-c/tests/GRPCClientTests.m index 0b72a75f3d5..a1bb835e5d4 100644 --- a/src/objective-c/tests/GRPCClientTests.m +++ b/src/objective-c/tests/GRPCClientTests.m @@ -37,7 +37,7 @@ #import #import #import -#import +#import #import #import #import diff --git a/src/objective-c/tests/InteropTests.m b/src/objective-c/tests/InteropTests.m index 2bd54e17142..592dca9dde5 100644 --- a/src/objective-c/tests/InteropTests.m +++ b/src/objective-c/tests/InteropTests.m @@ -37,7 +37,7 @@ #import #import -#import +#import #import #import #import diff --git a/src/objective-c/tests/InteropTestsLocalCleartext.m b/src/objective-c/tests/InteropTestsLocalCleartext.m index b41210f50f8..6e7624e2046 100644 --- a/src/objective-c/tests/InteropTestsLocalCleartext.m +++ b/src/objective-c/tests/InteropTestsLocalCleartext.m @@ -31,7 +31,7 @@ * */ -#import +#import #import "InteropTests.h" diff --git a/src/objective-c/tests/InteropTestsLocalSSL.m b/src/objective-c/tests/InteropTestsLocalSSL.m index 1479c5896c3..12007bbd681 100644 --- a/src/objective-c/tests/InteropTestsLocalSSL.m +++ b/src/objective-c/tests/InteropTestsLocalSSL.m @@ -31,7 +31,7 @@ * */ -#import +#import #import "InteropTests.h" diff --git a/src/objective-c/tests/InteropTestsRemote.m b/src/objective-c/tests/InteropTestsRemote.m index 70f84753bb6..b0a64a3caed 100644 --- a/src/objective-c/tests/InteropTestsRemote.m +++ b/src/objective-c/tests/InteropTestsRemote.m @@ -31,7 +31,7 @@ * */ -#import +#import #import "InteropTests.h" diff --git a/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m b/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m index 793b71a9bee..c8742b7fe8c 100644 --- a/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m +++ b/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m @@ -31,7 +31,7 @@ * */ -#import +#import #import #import diff --git a/src/objective-c/tests/Podfile b/src/objective-c/tests/Podfile index 462c6a8e0eb..5ada61c3d40 100644 --- a/src/objective-c/tests/Podfile +++ b/src/objective-c/tests/Podfile @@ -103,10 +103,14 @@ post_install do |installer| # Activate Cronet for the dedicated build configuration 'Cronet', which will be used solely by # the test target 'InteropTestsRemoteWithCronet' + # Activate GRPCCall+Tests functions for the dedicated build configuration 'Test', which will + # be used by all test targets using it. if target.name == 'gRPC' target.build_configurations.each do |config| if config.name == 'Cronet' - config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_COMPILE_WITH_CRONET=1' + config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_COMPILE_WITH_CRONET=1 GRPC_TEST_OBJC' + elsif config.name == 'Test' + config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_TEST_OBJC' end end end diff --git a/src/objective-c/tests/Tests.xcodeproj/project.pbxproj b/src/objective-c/tests/Tests.xcodeproj/project.pbxproj index 32b35ef333a..57e417c2a8a 100644 --- a/src/objective-c/tests/Tests.xcodeproj/project.pbxproj +++ b/src/objective-c/tests/Tests.xcodeproj/project.pbxproj @@ -125,8 +125,10 @@ 0A4F89D9C90E9C30990218F0 /* Pods.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = Pods.release.xcconfig; path = "Pods/Target Support Files/Pods/Pods.release.xcconfig"; sourceTree = ""; }; 0D2284C3DF7E57F0ED504E39 /* Pods-CoreCronetEnd2EndTests.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-CoreCronetEnd2EndTests.debug.xcconfig"; path = "Pods/Target Support Files/Pods-CoreCronetEnd2EndTests/Pods-CoreCronetEnd2EndTests.debug.xcconfig"; sourceTree = ""; }; 14B09A58FEE53A7A6B838920 /* Pods-InteropTestsLocalSSL.cronet.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsLocalSSL.cronet.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsLocalSSL/Pods-InteropTestsLocalSSL.cronet.xcconfig"; sourceTree = ""; }; + 1588C85DEAF7FC0ACDEA4C02 /* Pods-InteropTestsLocalCleartext.test.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsLocalCleartext.test.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsLocalCleartext/Pods-InteropTestsLocalCleartext.test.xcconfig"; sourceTree = ""; }; 17F60BF2871F6AF85FB3FA12 /* Pods-InteropTestsRemoteWithCronet.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsRemoteWithCronet.debug.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsRemoteWithCronet/Pods-InteropTestsRemoteWithCronet.debug.xcconfig"; sourceTree = ""; }; 20DFF2F3C97EF098FE5A3171 /* libPods-Tests.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-Tests.a"; sourceTree = BUILT_PRODUCTS_DIR; }; + 2B89F3037963E6EDDD48D8C3 /* Pods-InteropTestsRemoteWithCronet.test.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsRemoteWithCronet.test.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsRemoteWithCronet/Pods-InteropTestsRemoteWithCronet.test.xcconfig"; sourceTree = ""; }; 35F2B6BF3BAE8F0DC4AFD76E /* libPods.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = libPods.a; sourceTree = BUILT_PRODUCTS_DIR; }; 386712AEACF7C2190C4B8B3F /* Pods-CronetUnitTests.cronet.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-CronetUnitTests.cronet.xcconfig"; path = "Pods/Target Support Files/Pods-CronetUnitTests/Pods-CronetUnitTests.cronet.xcconfig"; sourceTree = ""; }; 3B0861FC805389C52DB260D4 /* Pods-RxLibraryUnitTests.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-RxLibraryUnitTests.release.xcconfig"; path = "Pods/Target Support Files/Pods-RxLibraryUnitTests/Pods-RxLibraryUnitTests.release.xcconfig"; sourceTree = ""; }; @@ -162,15 +164,22 @@ 63E240CD1B6C4E2B005F3B0E /* InteropTestsLocalSSL.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = InteropTestsLocalSSL.m; sourceTree = ""; }; 63E240CF1B6C63DC005F3B0E /* TestCertificates.bundle */ = {isa = PBXFileReference; lastKnownFileType = "wrapper.plug-in"; path = TestCertificates.bundle; sourceTree = ""; }; 64F68A9A6A63CC930DD30A6E /* Pods-CronetUnitTests.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-CronetUnitTests.debug.xcconfig"; path = "Pods/Target Support Files/Pods-CronetUnitTests/Pods-CronetUnitTests.debug.xcconfig"; sourceTree = ""; }; + 6793C9D019CB268C5BB491A2 /* Pods-CoreCronetEnd2EndTests.test.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-CoreCronetEnd2EndTests.test.xcconfig"; path = "Pods/Target Support Files/Pods-CoreCronetEnd2EndTests/Pods-CoreCronetEnd2EndTests.test.xcconfig"; sourceTree = ""; }; + 781089FAE980F51F88A3BE0B /* Pods-RxLibraryUnitTests.test.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-RxLibraryUnitTests.test.xcconfig"; path = "Pods/Target Support Files/Pods-RxLibraryUnitTests/Pods-RxLibraryUnitTests.test.xcconfig"; sourceTree = ""; }; 79C68EFFCB5533475D810B79 /* Pods-RxLibraryUnitTests.cronet.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-RxLibraryUnitTests.cronet.xcconfig"; path = "Pods/Target Support Files/Pods-RxLibraryUnitTests/Pods-RxLibraryUnitTests.cronet.xcconfig"; sourceTree = ""; }; 7A2E97E3F469CC2A758D77DE /* Pods-InteropTestsLocalSSL.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsLocalSSL.release.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsLocalSSL/Pods-InteropTestsLocalSSL.release.xcconfig"; sourceTree = ""; }; 9E9444C764F0FFF64A7EB58E /* libPods-InteropTestsRemoteWithCronet.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-InteropTestsRemoteWithCronet.a"; sourceTree = BUILT_PRODUCTS_DIR; }; + A0361771A855917162911180 /* Pods-Tests.test.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Tests.test.xcconfig"; path = "Pods/Target Support Files/Pods-Tests/Pods-Tests.test.xcconfig"; sourceTree = ""; }; A58BE6DF1C62D1739EBB2C78 /* libPods-RxLibraryUnitTests.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-RxLibraryUnitTests.a"; sourceTree = BUILT_PRODUCTS_DIR; }; + A6F832FCEFA6F6881E620F12 /* Pods-InteropTestsRemote.test.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsRemote.test.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsRemote/Pods-InteropTestsRemote.test.xcconfig"; sourceTree = ""; }; AA7CB64B4DD9915AE7C03163 /* Pods-InteropTestsLocalCleartext.cronet.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsLocalCleartext.cronet.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsLocalCleartext/Pods-InteropTestsLocalCleartext.cronet.xcconfig"; sourceTree = ""; }; AC414EF7A6BF76ED02B6E480 /* Pods-InteropTestsRemoteWithCronet.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsRemoteWithCronet.release.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsRemoteWithCronet/Pods-InteropTestsRemoteWithCronet.release.xcconfig"; sourceTree = ""; }; + B226619DC4E709E0FFFF94B8 /* Pods-CronetUnitTests.test.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-CronetUnitTests.test.xcconfig"; path = "Pods/Target Support Files/Pods-CronetUnitTests/Pods-CronetUnitTests.test.xcconfig"; sourceTree = ""; }; B94C27C06733CF98CE1B2757 /* Pods-AllTests.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AllTests.debug.xcconfig"; path = "Pods/Target Support Files/Pods-AllTests/Pods-AllTests.debug.xcconfig"; sourceTree = ""; }; C6134277D2EB8B380862A03F /* libPods-CronetUnitTests.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-CronetUnitTests.a"; sourceTree = BUILT_PRODUCTS_DIR; }; CAE086D5B470DA367D415AB0 /* libPods-AllTests.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-AllTests.a"; sourceTree = BUILT_PRODUCTS_DIR; }; + D13BEC8181B8E678A1B52F54 /* Pods-InteropTestsLocalSSL.test.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsLocalSSL.test.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsLocalSSL/Pods-InteropTestsLocalSSL.test.xcconfig"; sourceTree = ""; }; + DB1F4391AF69D20D38D74B67 /* Pods-AllTests.test.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-AllTests.test.xcconfig"; path = "Pods/Target Support Files/Pods-AllTests/Pods-AllTests.test.xcconfig"; sourceTree = ""; }; DBE059B4AC7A51919467EEC0 /* libPods-InteropTestsRemote.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-InteropTestsRemote.a"; sourceTree = BUILT_PRODUCTS_DIR; }; DBEDE45BDA60DF1E1C8950C0 /* libPods-InteropTestsLocalSSL.a */ = {isa = PBXFileReference; explicitFileType = archive.ar; includeInIndex = 0; path = "libPods-InteropTestsLocalSSL.a"; sourceTree = BUILT_PRODUCTS_DIR; }; DC3CA1D948F068E76957A861 /* Pods-InteropTestsRemote.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-InteropTestsRemote.debug.xcconfig"; path = "Pods/Target Support Files/Pods-InteropTestsRemote/Pods-InteropTestsRemote.debug.xcconfig"; sourceTree = ""; }; @@ -317,6 +326,15 @@ 64F68A9A6A63CC930DD30A6E /* Pods-CronetUnitTests.debug.xcconfig */, 386712AEACF7C2190C4B8B3F /* Pods-CronetUnitTests.cronet.xcconfig */, 02192CF1FF9534E3D18C65FC /* Pods-CronetUnitTests.release.xcconfig */, + DB1F4391AF69D20D38D74B67 /* Pods-AllTests.test.xcconfig */, + 6793C9D019CB268C5BB491A2 /* Pods-CoreCronetEnd2EndTests.test.xcconfig */, + B226619DC4E709E0FFFF94B8 /* Pods-CronetUnitTests.test.xcconfig */, + 1588C85DEAF7FC0ACDEA4C02 /* Pods-InteropTestsLocalCleartext.test.xcconfig */, + D13BEC8181B8E678A1B52F54 /* Pods-InteropTestsLocalSSL.test.xcconfig */, + A6F832FCEFA6F6881E620F12 /* Pods-InteropTestsRemote.test.xcconfig */, + 2B89F3037963E6EDDD48D8C3 /* Pods-InteropTestsRemoteWithCronet.test.xcconfig */, + 781089FAE980F51F88A3BE0B /* Pods-RxLibraryUnitTests.test.xcconfig */, + A0361771A855917162911180 /* Pods-Tests.test.xcconfig */, ); name = Pods; sourceTree = ""; @@ -1237,6 +1255,189 @@ /* End PBXTargetDependency section */ /* Begin XCBuildConfiguration section */ + 5E1228981E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + buildSettings = { + ALWAYS_SEARCH_USER_PATHS = NO; + CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x"; + CLANG_CXX_LIBRARY = "libc++"; + CLANG_ENABLE_MODULES = YES; + CLANG_ENABLE_OBJC_ARC = YES; + CLANG_WARN_BOOL_CONVERSION = YES; + CLANG_WARN_CONSTANT_CONVERSION = YES; + CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR; + CLANG_WARN_EMPTY_BODY = YES; + CLANG_WARN_ENUM_CONVERSION = YES; + CLANG_WARN_INT_CONVERSION = YES; + CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR; + CLANG_WARN_UNREACHABLE_CODE = YES; + CLANG_WARN__DUPLICATE_METHOD_MATCH = YES; + COPY_PHASE_STRIP = NO; + DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; + ENABLE_STRICT_OBJC_MSGSEND = YES; + GCC_C_LANGUAGE_STANDARD = gnu99; + GCC_DYNAMIC_NO_PIC = NO; + GCC_NO_COMMON_BLOCKS = YES; + GCC_OPTIMIZATION_LEVEL = 0; + GCC_PREPROCESSOR_DEFINITIONS = ( + "DEBUG=1", + "$(inherited)", + ); + GCC_SYMBOLS_PRIVATE_EXTERN = NO; + GCC_TREAT_WARNINGS_AS_ERRORS = YES; + GCC_WARN_64_TO_32_BIT_CONVERSION = YES; + GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR; + GCC_WARN_UNDECLARED_SELECTOR = YES; + GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE; + GCC_WARN_UNUSED_FUNCTION = YES; + GCC_WARN_UNUSED_VARIABLE = YES; + IPHONEOS_DEPLOYMENT_TARGET = 8.3; + MTL_ENABLE_DEBUG_INFO = YES; + ONLY_ACTIVE_ARCH = YES; + SDKROOT = iphoneos; + }; + name = Test; + }; + 5E1228991E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + baseConfigurationReference = A0361771A855917162911180 /* Pods-Tests.test.xcconfig */; + buildSettings = { + GCC_TREAT_WARNINGS_AS_ERRORS = YES; + PRODUCT_NAME = "$(TARGET_NAME)"; + SKIP_INSTALL = YES; + }; + name = Test; + }; + 5E12289A1E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + baseConfigurationReference = DB1F4391AF69D20D38D74B67 /* Pods-AllTests.test.xcconfig */; + buildSettings = { + FRAMEWORK_SEARCH_PATHS = ( + "$(SDKROOT)/Developer/Library/Frameworks", + "$(inherited)", + ); + GCC_PREPROCESSOR_DEFINITIONS = ( + "DEBUG=1", + "$(inherited)", + ); + INFOPLIST_FILE = Info.plist; + LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; + PRODUCT_NAME = "$(TARGET_NAME)"; + }; + name = Test; + }; + 5E12289B1E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + baseConfigurationReference = 781089FAE980F51F88A3BE0B /* Pods-RxLibraryUnitTests.test.xcconfig */; + buildSettings = { + DEBUG_INFORMATION_FORMAT = dwarf; + ENABLE_TESTABILITY = YES; + INFOPLIST_FILE = Info.plist; + IPHONEOS_DEPLOYMENT_TARGET = 9.0; + LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; + PRODUCT_BUNDLE_IDENTIFIER = io.grpc.RxLibraryUnitTests; + PRODUCT_NAME = "$(TARGET_NAME)"; + }; + name = Test; + }; + 5E12289C1E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + baseConfigurationReference = A6F832FCEFA6F6881E620F12 /* Pods-InteropTestsRemote.test.xcconfig */; + buildSettings = { + DEBUG_INFORMATION_FORMAT = dwarf; + ENABLE_TESTABILITY = YES; + INFOPLIST_FILE = Info.plist; + IPHONEOS_DEPLOYMENT_TARGET = 9.0; + LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; + PRODUCT_BUNDLE_IDENTIFIER = io.grpc.InteropTests; + PRODUCT_NAME = "$(TARGET_NAME)"; + }; + name = Test; + }; + 5E12289D1E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + baseConfigurationReference = D13BEC8181B8E678A1B52F54 /* Pods-InteropTestsLocalSSL.test.xcconfig */; + buildSettings = { + DEBUG_INFORMATION_FORMAT = dwarf; + ENABLE_TESTABILITY = YES; + INFOPLIST_FILE = Info.plist; + IPHONEOS_DEPLOYMENT_TARGET = 9.0; + LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; + PRODUCT_BUNDLE_IDENTIFIER = io.grpc.InteropTestsLocalSSL; + PRODUCT_NAME = "$(TARGET_NAME)"; + }; + name = Test; + }; + 5E12289E1E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + baseConfigurationReference = 1588C85DEAF7FC0ACDEA4C02 /* Pods-InteropTestsLocalCleartext.test.xcconfig */; + buildSettings = { + DEBUG_INFORMATION_FORMAT = dwarf; + ENABLE_TESTABILITY = YES; + INFOPLIST_FILE = Info.plist; + IPHONEOS_DEPLOYMENT_TARGET = 9.0; + LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; + PRODUCT_BUNDLE_IDENTIFIER = io.grpc.InteropTestsLocalCleartext; + PRODUCT_NAME = "$(TARGET_NAME)"; + }; + name = Test; + }; + 5E12289F1E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + baseConfigurationReference = 6793C9D019CB268C5BB491A2 /* Pods-CoreCronetEnd2EndTests.test.xcconfig */; + buildSettings = { + CLANG_ANALYZER_NONNULL = YES; + "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; + DEBUG_INFORMATION_FORMAT = dwarf; + ENABLE_TESTABILITY = YES; + INFOPLIST_FILE = CoreCronetEnd2EndTests/Info.plist; + IPHONEOS_DEPLOYMENT_TARGET = 9.3; + LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; + PRODUCT_BUNDLE_IDENTIFIER = io.grpc.CoreCronetEnd2EndTests; + PRODUCT_NAME = "$(TARGET_NAME)"; + USER_HEADER_SEARCH_PATHS = "$(inherited) \"${PODS_ROOT}/../../../..\""; + }; + name = Test; + }; + 5E1228A01E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + baseConfigurationReference = 2B89F3037963E6EDDD48D8C3 /* Pods-InteropTestsRemoteWithCronet.test.xcconfig */; + buildSettings = { + CLANG_ANALYZER_NONNULL = YES; + "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; + DEBUG_INFORMATION_FORMAT = dwarf; + ENABLE_TESTABILITY = YES; + GCC_PREPROCESSOR_DEFINITIONS = ( + "$(inherited)", + "COCOAPODS=1", + "$(inherited)", + "GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS=1", + ); + INFOPLIST_FILE = InteropTestsRemoteWithCronet/Info.plist; + IPHONEOS_DEPLOYMENT_TARGET = 9.3; + LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; + PRODUCT_BUNDLE_IDENTIFIER = io.grpc.InteropTestsRemoteWithCronet; + PRODUCT_NAME = "$(TARGET_NAME)"; + }; + name = Test; + }; + 5E1228A11E4D400F00E8504F /* Test */ = { + isa = XCBuildConfiguration; + baseConfigurationReference = B226619DC4E709E0FFFF94B8 /* Pods-CronetUnitTests.test.xcconfig */; + buildSettings = { + CLANG_ANALYZER_NONNULL = YES; + "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; + DEBUG_INFORMATION_FORMAT = dwarf; + ENABLE_TESTABILITY = YES; + INFOPLIST_FILE = CronetUnitTests/Info.plist; + IPHONEOS_DEPLOYMENT_TARGET = 9.3; + LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; + PRODUCT_BUNDLE_IDENTIFIER = io.grpc.CronetUnitTests; + PRODUCT_NAME = "$(TARGET_NAME)"; + USER_HEADER_SEARCH_PATHS = "\"${PODS_ROOT}/../../../..\" $(inherited)"; + }; + name = Test; + }; 5E8A5DAC1D3840B4000F8BC4 /* Debug */ = { isa = XCBuildConfiguration; baseConfigurationReference = 0D2284C3DF7E57F0ED504E39 /* Pods-CoreCronetEnd2EndTests.debug.xcconfig */; @@ -1770,6 +1971,7 @@ isa = XCConfigurationList; buildConfigurations = ( 5E8A5DAC1D3840B4000F8BC4 /* Debug */, + 5E12289F1E4D400F00E8504F /* Test */, 5EC3C7A71D4FC18C000330E2 /* Cronet */, 5E8A5DAD1D3840B4000F8BC4 /* Release */, ); @@ -1780,6 +1982,7 @@ isa = XCConfigurationList; buildConfigurations = ( 5EAD6D2C1E27047400002378 /* Debug */, + 5E1228A11E4D400F00E8504F /* Test */, 5EAD6D2D1E27047400002378 /* Cronet */, 5EAD6D2E1E27047400002378 /* Release */, ); @@ -1790,6 +1993,7 @@ isa = XCConfigurationList; buildConfigurations = ( 5EE84BF91D4717E40050C6CC /* Debug */, + 5E1228A01E4D400F00E8504F /* Test */, 5EC3C7A81D4FC18C000330E2 /* Cronet */, 5EE84BFA1D4717E40050C6CC /* Release */, ); @@ -1800,6 +2004,7 @@ isa = XCConfigurationList; buildConfigurations = ( 63423F4E1B150A5F006CF63C /* Debug */, + 5E12289A1E4D400F00E8504F /* Test */, 5EC3C7A21D4FC18C000330E2 /* Cronet */, 63423F4F1B150A5F006CF63C /* Release */, ); @@ -1810,6 +2015,7 @@ isa = XCConfigurationList; buildConfigurations = ( 635697D91B14FC11007A7283 /* Debug */, + 5E1228981E4D400F00E8504F /* Test */, 5EC3C7A01D4FC18C000330E2 /* Cronet */, 635697DA1B14FC11007A7283 /* Release */, ); @@ -1820,6 +2026,7 @@ isa = XCConfigurationList; buildConfigurations = ( 635697DC1B14FC11007A7283 /* Debug */, + 5E1228991E4D400F00E8504F /* Test */, 5EC3C7A11D4FC18C000330E2 /* Cronet */, 635697DD1B14FC11007A7283 /* Release */, ); @@ -1830,6 +2037,7 @@ isa = XCConfigurationList; buildConfigurations = ( 63DC841C1BE15179000708E8 /* Debug */, + 5E12289B1E4D400F00E8504F /* Test */, 5EC3C7A31D4FC18C000330E2 /* Cronet */, 63DC841D1BE15179000708E8 /* Release */, ); @@ -1840,6 +2048,7 @@ isa = XCConfigurationList; buildConfigurations = ( 63DC842C1BE15267000708E8 /* Debug */, + 5E12289C1E4D400F00E8504F /* Test */, 5EC3C7A41D4FC18C000330E2 /* Cronet */, 63DC842D1BE15267000708E8 /* Release */, ); @@ -1850,6 +2059,7 @@ isa = XCConfigurationList; buildConfigurations = ( 63DC843D1BE15294000708E8 /* Debug */, + 5E12289D1E4D400F00E8504F /* Test */, 5EC3C7A51D4FC18C000330E2 /* Cronet */, 63DC843E1BE15294000708E8 /* Release */, ); @@ -1860,6 +2070,7 @@ isa = XCConfigurationList; buildConfigurations = ( 63DC844C1BE152B5000708E8 /* Debug */, + 5E12289E1E4D400F00E8504F /* Test */, 5EC3C7A61D4FC18C000330E2 /* Cronet */, 63DC844D1BE152B5000708E8 /* Release */, ); diff --git a/src/objective-c/tests/Tests.xcodeproj/xcshareddata/xcschemes/AllTests.xcscheme b/src/objective-c/tests/Tests.xcodeproj/xcshareddata/xcschemes/AllTests.xcscheme index 5524a27ffde..7575609dd74 100644 --- a/src/objective-c/tests/Tests.xcodeproj/xcshareddata/xcschemes/AllTests.xcscheme +++ b/src/objective-c/tests/Tests.xcodeproj/xcshareddata/xcschemes/AllTests.xcscheme @@ -23,7 +23,7 @@ @@ -73,7 +73,7 @@ + buildConfiguration = "Test"> @@ -54,7 +54,7 @@ + buildConfiguration = "Test"> @@ -57,7 +57,7 @@ + buildConfiguration = "Test"> @@ -57,7 +57,7 @@ + buildConfiguration = "Test"> Date: Thu, 9 Feb 2017 16:55:51 -0800 Subject: [PATCH 05/26] Warning and control the rest of interfaces in GRPCCall+Tests as well --- .../internal_testing/GRPCCall+Tests.h | 9 +++++++++ .../internal_testing/GRPCCall+Tests.m | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h b/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h index 0325f5a192f..92c718a7493 100644 --- a/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h +++ b/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h @@ -45,6 +45,9 @@ * * Must be called before any gRPC call to that host is made. It's illegal to pass the same host to * more than one invocation of the methods of this category. + * + * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. + * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. */ + (void)useTestCertsPath:(NSString *)certsPath testName:(NSString *)testName @@ -55,12 +58,18 @@ * * Must be called before any gRPC call to that host is made. It's illegal to pass the same host to * more than one invocation of the methods of this category. + * + * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. + * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. */ + (void)useInsecureConnectionsForHost:(NSString *)host; /** * Resets all host configurations to their default values, and flushes all connections from the * cache. + * + * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. + * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. */ + (void)resetHostSettings; diff --git a/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m b/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m index 5109480bef8..40bd2a9c803 100644 --- a/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m +++ b/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m @@ -41,6 +41,7 @@ + (void)useTestCertsPath:(NSString *)certsPath testName:(NSString *)testName forHost:(NSString *)host { +#ifdef GRPC_TEST_OBJC if (!host || !certsPath || !testName) { [NSException raise:NSInvalidArgumentException format:@"host, path and name must be provided."]; } @@ -55,15 +56,32 @@ GRPCHost *hostConfig = [GRPCHost hostWithAddress:host]; [hostConfig setTLSPEMRootCerts:certs withPrivateKey:nil withCertChain:nil error:nil]; hostConfig.hostNameOverride = testName; +#else + NSLog(@"This function is for internal testing of gRPC only. " + "It is not part of gRPC's public interface. Do not use in production. " + "To enable, set the preprocessor flag GRPC_TEST_OBJC."); +#endif } + (void)useInsecureConnectionsForHost:(NSString *)host { +#ifdef GRPC_TEST_OBJC GRPCHost *hostConfig = [GRPCHost hostWithAddress:host]; hostConfig.secure = NO; +#else + NSLog(@"This function is for internal testing of gRPC only. " + "It is not part of gRPC's public interface. Do not use in production. " + "To enable, set the preprocessor flag GRPC_TEST_OBJC."); +#endif } + (void)resetHostSettings { +#ifdef GRPC_TEST_OBJC [GRPCHost resetAllHostSettings]; +#else + NSLog(@"This function is for internal testing of gRPC only. " + "It is not part of gRPC's public interface. Do not use in production. " + "To enable, set the preprocessor flag GRPC_TEST_OBJC."); +#endif } + (void)enableOpBatchLog:(BOOL)enabled { From 8e6aec5ed7d04a19311e8c0d260a02ed5c99dce2 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Fri, 10 Feb 2017 09:40:18 -0800 Subject: [PATCH 06/26] Separate/Restore public test interfaces and internal ones --- .../{internal_testing => }/GRPCCall+Tests.h | 27 +------- .../{internal_testing => }/GRPCCall+Tests.m | 43 +------------ .../internal_testing/GRPCCall+InternalTests.h | 57 +++++++++++++++++ .../internal_testing/GRPCCall+InternalTests.m | 61 +++++++++++++++++++ src/objective-c/tests/GRPCClientTests.m | 3 +- src/objective-c/tests/InteropTests.m | 3 +- .../tests/InteropTestsLocalCleartext.m | 3 +- src/objective-c/tests/InteropTestsLocalSSL.m | 3 +- src/objective-c/tests/InteropTestsRemote.m | 3 +- .../InteropTestsRemoteWithCronet.m | 3 +- src/objective-c/tests/Podfile | 2 +- 11 files changed, 133 insertions(+), 75 deletions(-) rename src/objective-c/GRPCClient/{internal_testing => }/GRPCCall+Tests.h (69%) rename src/objective-c/GRPCClient/{internal_testing => }/GRPCCall+Tests.m (64%) create mode 100644 src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.h create mode 100644 src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.m diff --git a/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h b/src/objective-c/GRPCClient/GRPCCall+Tests.h similarity index 69% rename from src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h rename to src/objective-c/GRPCClient/GRPCCall+Tests.h index 92c718a7493..184ad09c5c8 100644 --- a/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.h +++ b/src/objective-c/GRPCClient/GRPCCall+Tests.h @@ -31,7 +31,7 @@ * */ -#import "../GRPCCall.h" +#import "GRPCCall.h" /** * Methods to let tune down the security of gRPC connections for specific hosts. These shouldn't be @@ -45,9 +45,6 @@ * * Must be called before any gRPC call to that host is made. It's illegal to pass the same host to * more than one invocation of the methods of this category. - * - * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. - * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. */ + (void)useTestCertsPath:(NSString *)certsPath testName:(NSString *)testName @@ -58,34 +55,12 @@ * * Must be called before any gRPC call to that host is made. It's illegal to pass the same host to * more than one invocation of the methods of this category. - * - * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. - * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. */ + (void)useInsecureConnectionsForHost:(NSString *)host; /** * Resets all host configurations to their default values, and flushes all connections from the * cache. - * - * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. - * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. */ + (void)resetHostSettings; - -/** - * Enables logging of op batches. Memory consumption increases as more ops are logged. - * - * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. - * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. - */ -+ (void)enableOpBatchLog:(BOOL)enabled; - -/** - * Obtain the logged op batches. Invoking this method will clean the log. - * - * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. - * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. - */ -+ (NSArray *)obtainAndCleanOpBatchLog; @end diff --git a/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m b/src/objective-c/GRPCClient/GRPCCall+Tests.m similarity index 64% rename from src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m rename to src/objective-c/GRPCClient/GRPCCall+Tests.m index 40bd2a9c803..656cba8fec6 100644 --- a/src/objective-c/GRPCClient/internal_testing/GRPCCall+Tests.m +++ b/src/objective-c/GRPCClient/GRPCCall+Tests.m @@ -33,15 +33,13 @@ #import "GRPCCall+Tests.h" -#import "../private/GRPCHost.h" -#import "../private/GRPCOpBatchLog.h" +#import "private/GRPCHost.h" @implementation GRPCCall (Tests) + (void)useTestCertsPath:(NSString *)certsPath testName:(NSString *)testName forHost:(NSString *)host { -#ifdef GRPC_TEST_OBJC if (!host || !certsPath || !testName) { [NSException raise:NSInvalidArgumentException format:@"host, path and name must be provided."]; } @@ -56,53 +54,14 @@ GRPCHost *hostConfig = [GRPCHost hostWithAddress:host]; [hostConfig setTLSPEMRootCerts:certs withPrivateKey:nil withCertChain:nil error:nil]; hostConfig.hostNameOverride = testName; -#else - NSLog(@"This function is for internal testing of gRPC only. " - "It is not part of gRPC's public interface. Do not use in production. " - "To enable, set the preprocessor flag GRPC_TEST_OBJC."); -#endif } + (void)useInsecureConnectionsForHost:(NSString *)host { -#ifdef GRPC_TEST_OBJC GRPCHost *hostConfig = [GRPCHost hostWithAddress:host]; hostConfig.secure = NO; -#else - NSLog(@"This function is for internal testing of gRPC only. " - "It is not part of gRPC's public interface. Do not use in production. " - "To enable, set the preprocessor flag GRPC_TEST_OBJC."); -#endif } + (void)resetHostSettings { -#ifdef GRPC_TEST_OBJC [GRPCHost resetAllHostSettings]; -#else - NSLog(@"This function is for internal testing of gRPC only. " - "It is not part of gRPC's public interface. Do not use in production. " - "To enable, set the preprocessor flag GRPC_TEST_OBJC."); -#endif } - -+ (void)enableOpBatchLog:(BOOL)enabled { -#ifdef GRPC_TEST_OBJC - [GRPCOpBatchLog enableOpBatchLog:enabled]; -#else - NSLog(@"This function is for internal testing of gRPC only. " - "It is not part of gRPC's public interface. Do not use in production. " - "To enable, set the preprocessor flag GRPC_TEST_OBJC."); -#endif -} - -+ (NSArray *)obtainAndCleanOpBatchLog { -#ifdef GRPC_TEST_OBJC - return [GRPCOpBatchLog obtainAndCleanOpBatchLog]; -#else - NSLog(@"This function is for internal testing of gRPC only. " - "It is not part of gRPC's public interface. Do not use in production. " - "To enable, set the preprocessor flag GRPC_TEST_OBJC."); - return nil; -#endif -} - @end diff --git a/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.h b/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.h new file mode 100644 index 00000000000..6c12ce6355e --- /dev/null +++ b/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.h @@ -0,0 +1,57 @@ +/* + * + * Copyright 2017, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#import "../GRPCCall.h" + +/** + * Methods used for gRPC internal tests. DO NOT USE. + */ +@interface GRPCCall (InternalTests) + +/** + * Enables logging of op batches. Memory consumption increases as more ops are logged. + * + * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. + * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. + */ ++ (void)enableOpBatchLog:(BOOL)enabled; + +/** + * Obtain the logged op batches. Invoking this method will clean the log. + * + * This function is for internal testing of gRPC only. It is not part of gRPC's public interface. + * Do not use in production. To enable, set the preprocessor flag GRPC_TEST_OBJC. + */ ++ (NSArray *)obtainAndCleanOpBatchLog; + +@end diff --git a/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.m b/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.m new file mode 100644 index 00000000000..43c1c078316 --- /dev/null +++ b/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.m @@ -0,0 +1,61 @@ +/* + * + * Copyright 2017, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#import "GRPCCall+InternalTests.h" + +#import "../private/GRPCOpBatchLog.h" + +@implementation GRPCCall (InternalTests) + ++ (void)enableOpBatchLog:(BOOL)enabled { +#ifdef GRPC_TEST_OBJC + [GRPCOpBatchLog enableOpBatchLog:enabled]; +#else + NSLog(@"This function is for internal testing of gRPC only. " + "It is not part of gRPC's public interface. Do not use in production. " + "To enable, set the preprocessor flag GRPC_TEST_OBJC."); +#endif +} + ++ (NSArray *)obtainAndCleanOpBatchLog { +#ifdef GRPC_TEST_OBJC + return [GRPCOpBatchLog obtainAndCleanOpBatchLog]; +#else + NSLog(@"This function is for internal testing of gRPC only. " + "It is not part of gRPC's public interface. Do not use in production. " + "To enable, set the preprocessor flag GRPC_TEST_OBJC."); + return nil; +#endif +} + +@end diff --git a/src/objective-c/tests/GRPCClientTests.m b/src/objective-c/tests/GRPCClientTests.m index a1bb835e5d4..76c15003f60 100644 --- a/src/objective-c/tests/GRPCClientTests.m +++ b/src/objective-c/tests/GRPCClientTests.m @@ -37,7 +37,8 @@ #import #import #import -#import +#import +#import #import #import #import diff --git a/src/objective-c/tests/InteropTests.m b/src/objective-c/tests/InteropTests.m index 592dca9dde5..4e5a5287c2f 100644 --- a/src/objective-c/tests/InteropTests.m +++ b/src/objective-c/tests/InteropTests.m @@ -37,7 +37,8 @@ #import #import -#import +#import +#import #import #import #import diff --git a/src/objective-c/tests/InteropTestsLocalCleartext.m b/src/objective-c/tests/InteropTestsLocalCleartext.m index 6e7624e2046..0b8a0e47756 100644 --- a/src/objective-c/tests/InteropTestsLocalCleartext.m +++ b/src/objective-c/tests/InteropTestsLocalCleartext.m @@ -31,7 +31,8 @@ * */ -#import +#import +#import #import "InteropTests.h" diff --git a/src/objective-c/tests/InteropTestsLocalSSL.m b/src/objective-c/tests/InteropTestsLocalSSL.m index 12007bbd681..d974c2db27b 100644 --- a/src/objective-c/tests/InteropTestsLocalSSL.m +++ b/src/objective-c/tests/InteropTestsLocalSSL.m @@ -31,7 +31,8 @@ * */ -#import +#import +#import #import "InteropTests.h" diff --git a/src/objective-c/tests/InteropTestsRemote.m b/src/objective-c/tests/InteropTestsRemote.m index b0a64a3caed..ad7efebc81c 100644 --- a/src/objective-c/tests/InteropTestsRemote.m +++ b/src/objective-c/tests/InteropTestsRemote.m @@ -31,7 +31,8 @@ * */ -#import +#import +#import #import "InteropTests.h" diff --git a/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m b/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m index c8742b7fe8c..660865abb44 100644 --- a/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m +++ b/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m @@ -31,7 +31,8 @@ * */ -#import +#import +#import #import #import diff --git a/src/objective-c/tests/Podfile b/src/objective-c/tests/Podfile index 5ada61c3d40..941031ff88e 100644 --- a/src/objective-c/tests/Podfile +++ b/src/objective-c/tests/Podfile @@ -103,7 +103,7 @@ post_install do |installer| # Activate Cronet for the dedicated build configuration 'Cronet', which will be used solely by # the test target 'InteropTestsRemoteWithCronet' - # Activate GRPCCall+Tests functions for the dedicated build configuration 'Test', which will + # Activate GRPCCall+InternalTests functions for the dedicated build configuration 'Test', which will # be used by all test targets using it. if target.name == 'gRPC' target.build_configurations.each do |config| From 1fb35b5348fa737cca189ef773a7909ef62d6469 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 7 Mar 2017 17:42:25 -0800 Subject: [PATCH 07/26] Add comments to packet coalescing test --- src/objective-c/tests/InteropTests.m | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/objective-c/tests/InteropTests.m b/src/objective-c/tests/InteropTests.m index 4e5a5287c2f..c9941fd37de 100644 --- a/src/objective-c/tests/InteropTests.m +++ b/src/objective-c/tests/InteropTests.m @@ -164,11 +164,15 @@ expectedResponse.payload.body = [NSMutableData dataWithLength:10]; XCTAssertEqualObjects(response, expectedResponse); + // The test is a success if there is a batch of exactly 3 ops (SEND_INITIAL_METADATA, + // SEND_MESSAGE, SEND_CLOSE_FROM_CLIENT). Without packet coalescing each batch of ops contains + // only one op. NSArray *opBatches = [GRPCCall obtainAndCleanOpBatchLog]; + const NSInteger kExpectedOpBatchSize = 3; for (NSObject *o in opBatches) { if ([o isKindOfClass:[NSArray class]]) { NSArray *batch = (NSArray *)o; - if ([batch count] == 3) { + if ([batch count] == kExpectedOpBatchSize) { [expectation fulfill]; break; } From 7e49365f920a874b5133cb05884076b0f556e7ac Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 7 Mar 2017 17:47:02 -0800 Subject: [PATCH 08/26] Remove Cronet test cherry-pick --- .../InteropTestsRemoteWithCronet.m | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m b/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m index 660865abb44..a6720e41dce 100644 --- a/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m +++ b/src/objective-c/tests/InteropTestsRemoteWithCronet/InteropTestsRemoteWithCronet.m @@ -47,14 +47,6 @@ static NSString * const kRemoteSSLHost = @"grpc-test.sandbox.googleapis.com"; @implementation InteropTestsRemoteWithCronet -+ (void)setUp { - // Cronet setup - [Cronet setHttp2Enabled:YES]; - [Cronet start]; - [GRPCCall useCronetWithEngine:[Cronet getGlobalEngine]]; - [Cronet startNetLogToFile:@"Documents/cronet_netlog.json" logBytes:YES]; -} - + (NSString *)host { return kRemoteSSLHost; } From 43730581800577cf0e02b66677fe73841bdbecfe Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 7 Mar 2017 17:49:13 -0800 Subject: [PATCH 09/26] Add comment to logging operations batch --- src/objective-c/GRPCClient/private/GRPCWrappedCall.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m index 4476a41bd97..46e9fee7e1f 100644 --- a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m +++ b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m @@ -276,6 +276,8 @@ } - (void)startBatchWithOperations:(NSArray *)operations errorHandler:(void (^)())errorHandler { + // Keep logs of op batches when we are running tests. Disabled when in production for improved + // performance. #ifdef GRPC_TEST_OBJC [GRPCOpBatchLog addOpBatchToLog:operations]; #endif From 016d1085fc950023f232c4a8f93723fd591c6baa Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 7 Mar 2017 18:26:42 -0800 Subject: [PATCH 10/26] Eliminate magic number 6 --- src/objective-c/GRPCClient/GRPCCall.m | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 1a8fc2e2ef6..1cda827e782 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -46,6 +46,11 @@ #import "private/NSDictionary+GRPC.h" #import "private/NSError+GRPC.h" +// At most 6 ops can be in an op batch for a client: SEND_INITIAL_METADATA, +// SEND_MESSAGE, SEND_CLOSE_FROM_CLIENT, RECV_INITIAL_METADATA, RECV_MESSAGE, +// and RECV_STATUS_ON_CLIENT. +NSInteger kMaxClientBatch = 6; + NSString * const kGRPCHeadersKey = @"io.grpc.HeadersKey"; NSString * const kGRPCTrailersKey = @"io.grpc.TrailersKey"; static NSMutableDictionary *callFlags; @@ -165,7 +170,7 @@ static NSMutableDictionary *callFlags; if ([requestWriter isKindOfClass:[GRXImmediateSingleWriter class]]) { _unaryCall = YES; - _unaryOpBatch = [NSMutableArray arrayWithCapacity:6]; + _unaryOpBatch = [NSMutableArray arrayWithCapacity:kMaxClientBatch]; } } return self; From d5bac0d38dfba3d9efec45ecbdf9c45678be13f9 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 7 Mar 2017 18:44:19 -0800 Subject: [PATCH 11/26] Add comment and fix a trivial --- src/objective-c/GRPCClient/GRPCCall.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 1cda827e782..da861f6fca4 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -274,7 +274,7 @@ static NSMutableDictionary *callFlags; // TODO(jcanizales): Add error handlers for async failures GRPCOpSendMetadata *op = [[GRPCOpSendMetadata alloc] initWithMetadata:headers flags:[GRPCCall callFlagsForHost:_host path:_path] - handler:nil]; + handler:nil]; // No clean-up needed after SEND_INITIAL_METADATA if (!_unaryCall) { [_wrappedCall startBatchWithOperations:@[op]]; } else { @@ -331,7 +331,7 @@ static NSMutableDictionary *callFlags; // 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 { - if (!_unaryOpBatch) { + if (!_unaryCall) { [_wrappedCall startBatchWithOperations:@[[[GRPCOpSendClose alloc] init]] errorHandler:errorHandler]; } else { From 35653011a32eace36c82eb7224faee2be15a09de Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 7 Mar 2017 18:54:24 -0800 Subject: [PATCH 12/26] Add comments --- src/objective-c/GRPCClient/GRPCCall.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index da861f6fca4..ff474650122 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -286,6 +286,8 @@ static NSMutableDictionary *callFlags; // Only called from the call queue. The error handler will be called from the // network queue if the write didn't succeed. +// If the call is a unary call, parameter \a errorHandler will be ignored and +// the error handler of GRPCOpSendClose will be executed in case of error. - (void)writeMessage:(NSData *)message withErrorHandler:(void (^)())errorHandler { __weak GRPCCall *weakSelf = self; From 33c7e8b87899f7e0241e79981b406458a114e841 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 7 Mar 2017 19:00:13 -0800 Subject: [PATCH 13/26] remove redundant if statement --- src/objective-c/GRPCClient/private/GRPCOpBatchLog.m | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m index e5e7100ddf0..4699b8e6b3e 100644 --- a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m +++ b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m @@ -53,9 +53,7 @@ NSMutableArray *opBatchLog = nil; + (void)addOpBatchToLog:(NSArray *)batch { @synchronized (opBatchLog) { - if (opBatchLog) { - [opBatchLog addObject:batch]; - } + [opBatchLog addObject:batch]; } } From 8ce266449705c13fc6645b4e55e4f75411fb622c Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 7 Mar 2017 19:03:25 -0800 Subject: [PATCH 14/26] remove another redundant if statement --- src/objective-c/GRPCClient/private/GRPCOpBatchLog.m | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m index 4699b8e6b3e..c172c3c988c 100644 --- a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m +++ b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m @@ -59,13 +59,9 @@ NSMutableArray *opBatchLog = nil; + (NSArray *)obtainAndCleanOpBatchLog { @synchronized (opBatchLog) { - if (opBatchLog) { - NSArray *out = opBatchLog; - opBatchLog = [NSMutableArray array]; - return out; - } else { - return nil; - } + NSArray *out = opBatchLog; + opBatchLog = [NSMutableArray array]; + return out; } } From 34894e4b1c46697a5b484d5f6b7734ea8795d149 Mon Sep 17 00:00:00 2001 From: Yuxuan Li Date: Wed, 8 Feb 2017 17:57:24 -0800 Subject: [PATCH 15/26] Implemented stream coalescing design (RFC:https://github.com/grpc/proposal/pull/3). Add necessary microbenchmarks to show reduced writes/iter for short streams. Add necessary end2end test to test out newly added apis. Filter those tests using *WithCoalescingApi*. --- include/grpc++/impl/codegen/async_stream.h | 205 ++++++++++- include/grpc++/impl/codegen/call.h | 46 ++- include/grpc++/impl/codegen/client_context.h | 13 +- include/grpc++/impl/codegen/sync_stream.h | 106 ++++-- include/grpc/impl/codegen/grpc_types.h | 19 +- .../chttp2/transport/chttp2_transport.c | 37 +- src/cpp/client/client_context.cc | 7 +- test/cpp/end2end/async_end2end_test.cc | 319 ++++++++++++++++++ test/cpp/end2end/end2end_test.cc | 85 +++++ test/cpp/end2end/test_service_impl.cc | 22 +- test/cpp/end2end/test_service_impl.h | 2 + test/cpp/microbenchmarks/bm_fullstack.cc | 203 ++++++++++- 12 files changed, 989 insertions(+), 75 deletions(-) diff --git a/include/grpc++/impl/codegen/async_stream.h b/include/grpc++/impl/codegen/async_stream.h index 1a5cbbd45da..f21139618f5 100644 --- a/include/grpc++/impl/codegen/async_stream.h +++ b/include/grpc++/impl/codegen/async_stream.h @@ -101,6 +101,39 @@ class AsyncWriterInterface { /// \param[in] msg The message to be written. /// \param[in] tag The tag identifying the operation. virtual void Write(const W& msg, void* tag) = 0; + + /// Request the writing of \a msg using WriteOptions \a options with + /// identifying tag \a tag. + /// + /// Only one write may be outstanding at any given time. This means that + /// after calling Write, one must wait to receive \a tag from the completion + /// queue BEFORE calling Write again. + /// WriteOptions \a options is used to set the write options of this message. + /// This is thread-safe with respect to \a Read + /// + /// \param[in] msg The message to be written. + /// \param[in] options The WriteOptions to be used to write this message. + /// \param[in] tag The tag identifying the operation. + virtual void Write(const W& msg, WriteOptions options, void* tag) = 0; + + /// Request the writing of \a msg and coalesce it with the writing + /// of trailing metadata, using WriteOptions \a options with identifying tag + /// \a tag. + /// + /// For client, WriteLast is equivalent of performing Write and WritesDone in + /// a single step. + /// For server, WriteLast buffers the \a msg. The writing of \a msg is held + /// until Finish is called, where \a msg and trailing metadata are coalesced + /// and write is initiated. Note that WriteLast can only buffer \a msg up to + /// the flow control window size. If \a msg size is larger than the window + /// size, it will be sent on wire without buffering. + /// + /// \param[in] msg The message to be written. + /// \param[in] options The WriteOptions to be used to write this message. + /// \param[in] tag The tag identifying the operation. + void WriteLast(const W& msg, WriteOptions options, void* tag) { + Write(msg, options.set_last_message(), tag); + } }; template @@ -183,11 +216,17 @@ class ClientAsyncWriter final : public ClientAsyncWriterInterface { : context_(context), call_(channel->CreateCall(method, context, cq)) { finish_ops_.RecvMessage(response); finish_ops_.AllowNoMessage(); - - init_ops_.set_output_tag(tag); - init_ops_.SendInitialMetadata(context->send_initial_metadata_, - context->initial_metadata_flags()); - call_.PerformOps(&init_ops_); + // if corked bit is set in context, we buffer up the initial metadata to + // coalesce with later message to be sent. No op is performed. + if (context_->initial_metadata_corked_) { + write_ops_.SendInitialMetadata(context->send_initial_metadata_, + context->initial_metadata_flags()); + } else { + init_ops_.set_output_tag(tag); + init_ops_.SendInitialMetadata(context->send_initial_metadata_, + context->initial_metadata_flags()); + call_.PerformOps(&init_ops_); + } } void ReadInitialMetadata(void* tag) override { @@ -205,6 +244,17 @@ class ClientAsyncWriter final : public ClientAsyncWriterInterface { call_.PerformOps(&write_ops_); } + void Write(const W& msg, WriteOptions options, void* tag) { + write_ops_.set_output_tag(tag); + if (options.is_last_message()) { + options.set_buffer_hint(); + write_ops_.ClientSendClose(); + } + // TODO(ctiller): don't assert + GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); + call_.PerformOps(&write_ops_); + } + void WritesDone(void* tag) override { writes_done_ops_.set_output_tag(tag); writes_done_ops_.ClientSendClose(); @@ -225,7 +275,8 @@ class ClientAsyncWriter final : public ClientAsyncWriterInterface { Call call_; CallOpSet init_ops_; CallOpSet meta_ops_; - CallOpSet write_ops_; + CallOpSet + write_ops_; CallOpSet writes_done_ops_; CallOpSet @@ -253,10 +304,17 @@ class ClientAsyncReaderWriter final const RpcMethod& method, ClientContext* context, void* tag) : context_(context), call_(channel->CreateCall(method, context, cq)) { - init_ops_.set_output_tag(tag); - init_ops_.SendInitialMetadata(context->send_initial_metadata_, - context->initial_metadata_flags()); - call_.PerformOps(&init_ops_); + if (context_->initial_metadata_corked_) { + // if corked bit is set in context, we buffer up the initial metadata to + // coalesce with later message to be sent. No op is performed. + write_ops_.SendInitialMetadata(context->send_initial_metadata_, + context->initial_metadata_flags()); + } else { + init_ops_.set_output_tag(tag); + init_ops_.SendInitialMetadata(context->send_initial_metadata_, + context->initial_metadata_flags()); + call_.PerformOps(&init_ops_); + } } void ReadInitialMetadata(void* tag) override { @@ -283,10 +341,21 @@ class ClientAsyncReaderWriter final call_.PerformOps(&write_ops_); } + void Write(const W& msg, WriteOptions options, void* tag) { + write_ops_.set_output_tag(tag); + if (options.is_last_message()) { + options.set_buffer_hint(); + write_ops_.ClientSendClose(); + } + // TODO(ctiller): don't assert + GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); + call_.PerformOps(&write_ops_); + } + void WritesDone(void* tag) override { - writes_done_ops_.set_output_tag(tag); - writes_done_ops_.ClientSendClose(); - call_.PerformOps(&writes_done_ops_); + write_ops_.set_output_tag(tag); + write_ops_.ClientSendClose(); + call_.PerformOps(&write_ops_); } void Finish(Status* status, void* tag) override { @@ -304,8 +373,8 @@ class ClientAsyncReaderWriter final CallOpSet init_ops_; CallOpSet meta_ops_; CallOpSet> read_ops_; - CallOpSet write_ops_; - CallOpSet writes_done_ops_; + CallOpSet + write_ops_; CallOpSet finish_ops_; }; @@ -395,6 +464,20 @@ class ServerAsyncWriterInterface : public ServerAsyncStreamingInterface, public AsyncWriterInterface { public: virtual void Finish(const Status& status, void* tag) = 0; + + /// Request the writing of \a msg and coalesce it with trailing metadata which + /// contains \a status, using WriteOptions options with identifying tag \a + /// tag. + /// + /// WriteAndFinish is equivalent of performing WriteLast and Finish in a + /// single step. + /// + /// \param[in] msg The message to be written. + /// \param[in] options The WriteOptions to be used to write this message. + /// \param[in] status The Status that server returns to client. + /// \param[in] tag The tag identifying the operation. + virtual void WriteAndFinish(const W& msg, WriteOptions options, + const Status& status, void* tag) = 0; }; template @@ -431,6 +514,42 @@ class ServerAsyncWriter final : public ServerAsyncWriterInterface { call_.PerformOps(&write_ops_); } + void Write(const W& msg, WriteOptions options, void* tag) override { + write_ops_.set_output_tag(tag); + if (options.is_last_message()) { + options.set_buffer_hint(); + } + + if (!ctx_->sent_initial_metadata_) { + write_ops_.SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); + if (ctx_->compression_level_set()) { + write_ops_.set_compression_level(ctx_->compression_level()); + } + ctx_->sent_initial_metadata_ = true; + } + // TODO(ctiller): don't assert + GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); + call_.PerformOps(&write_ops_); + } + + void WriteAndFinish(const W& msg, WriteOptions options, const Status& status, + void* tag) override { + write_ops_.set_output_tag(tag); + if (!ctx_->sent_initial_metadata_) { + write_ops_.SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); + if (ctx_->compression_level_set()) { + write_ops_.set_compression_level(ctx_->compression_level()); + } + ctx_->sent_initial_metadata_ = true; + } + options.set_buffer_hint(); + GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); + write_ops_.ServerSendStatus(ctx_->trailing_metadata_, status); + call_.PerformOps(&write_ops_); + } + void Finish(const Status& status, void* tag) override { finish_ops_.set_output_tag(tag); if (!ctx_->sent_initial_metadata_) { @@ -451,7 +570,9 @@ class ServerAsyncWriter final : public ServerAsyncWriterInterface { Call call_; ServerContext* ctx_; CallOpSet meta_ops_; - CallOpSet write_ops_; + CallOpSet + write_ops_; CallOpSet finish_ops_; }; @@ -462,6 +583,20 @@ class ServerAsyncReaderWriterInterface : public ServerAsyncStreamingInterface, public AsyncReaderInterface { public: virtual void Finish(const Status& status, void* tag) = 0; + + /// Request the writing of \a msg and coalesce it with trailing metadata which + /// contains \a status, using WriteOptions options with identifying tag \a + /// tag. + /// + /// WriteAndFinish is equivalent of performing WriteLast and Finish in a + /// single step. + /// + /// \param[in] msg The message to be written. + /// \param[in] options The WriteOptions to be used to write this message. + /// \param[in] status The Status that server returns to client. + /// \param[in] tag The tag identifying the operation. + virtual void WriteAndFinish(const W& msg, WriteOptions options, + const Status& status, void* tag) = 0; }; template @@ -505,6 +640,40 @@ class ServerAsyncReaderWriter final call_.PerformOps(&write_ops_); } + void Write(const W& msg, WriteOptions options, void* tag) override { + write_ops_.set_output_tag(tag); + if (options.is_last_message()) { + options.set_buffer_hint(); + } + if (!ctx_->sent_initial_metadata_) { + write_ops_.SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); + if (ctx_->compression_level_set()) { + write_ops_.set_compression_level(ctx_->compression_level()); + } + ctx_->sent_initial_metadata_ = true; + } + GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); + call_.PerformOps(&write_ops_); + } + + void WriteAndFinish(const W& msg, WriteOptions options, const Status& status, + void* tag) override { + write_ops_.set_output_tag(tag); + if (!ctx_->sent_initial_metadata_) { + write_ops_.SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); + if (ctx_->compression_level_set()) { + write_ops_.set_compression_level(ctx_->compression_level()); + } + ctx_->sent_initial_metadata_ = true; + } + options.set_buffer_hint(); + GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); + write_ops_.ServerSendStatus(ctx_->trailing_metadata_, status); + call_.PerformOps(&write_ops_); + } + void Finish(const Status& status, void* tag) override { finish_ops_.set_output_tag(tag); if (!ctx_->sent_initial_metadata_) { @@ -528,7 +697,9 @@ class ServerAsyncReaderWriter final ServerContext* ctx_; CallOpSet meta_ops_; CallOpSet> read_ops_; - CallOpSet write_ops_; + CallOpSet + write_ops_; CallOpSet finish_ops_; }; diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index 19a5ca2b2ee..f6638471d24 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -141,6 +141,47 @@ class WriteOptions { /// \sa GRPC_WRITE_BUFFER_HINT inline bool get_buffer_hint() const { return GetBit(GRPC_WRITE_BUFFER_HINT); } + /// corked bit: aliases set_buffer_hint currently, with the intent that + /// set_buffer_hint will be removed in the future + inline WriteOptions& set_corked() { + SetBit(GRPC_WRITE_BUFFER_HINT); + return *this; + } + + inline WriteOptions& clear_corked() { + ClearBit(GRPC_WRITE_BUFFER_HINT); + return *this; + } + + inline bool is_corked() const { return GetBit(GRPC_WRITE_BUFFER_HINT); } + + /// last-message bit: indicates this is the last message in a stream + /// client-side: makes Write the equivalent of performing Write, WritesDone + /// in a single step + /// server-side: hold the Write until the service handler returns (sync api) + /// or until Finish is called (async api) + /// + /// \sa GRPC_WRITE_LAST_MESSAGE + inline WriteOptions& set_last_message() { + SetBit(GRPC_WRITE_LAST_MESSAGE); + return *this; + } + + /// Clears flag indicating that this is the last message in a stream, + /// disabling coalescing. + /// + /// \sa GRPC_WRITE_LAST_MESSAGE + inline WriteOptions& clear_last_messsage() { + ClearBit(GRPC_WRITE_LAST_MESSAGE); + return *this; + } + + /// Get value for the flag indicating that this is the last message, and + /// should be coalesced with trailing metadata. + /// + /// \sa GRPC_WRITE_LAST_MESSAGE + bool is_last_message() const { return GetBit(GRPC_WRITE_LAST_MESSAGE); } + WriteOptions& operator=(const WriteOptions& rhs) { flags_ = rhs.flags_; return *this; @@ -224,7 +265,7 @@ class CallOpSendMessage { /// after use. template Status SendMessage(const M& message, - const WriteOptions& options) GRPC_MUST_USE_RESULT; + WriteOptions options) GRPC_MUST_USE_RESULT; template Status SendMessage(const M& message) GRPC_MUST_USE_RESULT; @@ -252,8 +293,7 @@ class CallOpSendMessage { }; template -Status CallOpSendMessage::SendMessage(const M& message, - const WriteOptions& options) { +Status CallOpSendMessage::SendMessage(const M& message, WriteOptions options) { write_options_ = options; return SerializationTraits::Serialize(message, &send_buf_, &own_buf_); } diff --git a/include/grpc++/impl/codegen/client_context.h b/include/grpc++/impl/codegen/client_context.h index b91c7f65d43..cc48b9c3f57 100644 --- a/include/grpc++/impl/codegen/client_context.h +++ b/include/grpc++/impl/codegen/client_context.h @@ -281,6 +281,15 @@ class ClientContext { /// \param algorithm The compression algorithm used for the client call. void set_compression_algorithm(grpc_compression_algorithm algorithm); + /// Flag whether the initial metadata should be \a corked + /// + /// If \a corked is true, then the initial metadata will be colasced with the + /// write of first message in the stream. + /// + /// \param corked The flag indicating whether the initial metadata is to be + /// corked or not. + void sent_initial_metadata_corked(bool corked); + /// Return the peer uri in a string. /// /// \warning This value is never authenticated or subject to any security @@ -357,7 +366,8 @@ class ClientContext { (cacheable_ ? GRPC_INITIAL_METADATA_CACHEABLE_REQUEST : 0) | (wait_for_ready_explicitly_set_ ? GRPC_INITIAL_METADATA_WAIT_FOR_READY_EXPLICITLY_SET - : 0); + : 0) | + (initial_metadata_corked_ ? GRPC_INITIAL_METADATA_CORKED : 0); } grpc::string authority() { return authority_; } @@ -384,6 +394,7 @@ class ClientContext { PropagationOptions propagation_options_; grpc_compression_algorithm compression_algorithm_; + bool initial_metadata_corked_; }; } // namespace grpc diff --git a/include/grpc++/impl/codegen/sync_stream.h b/include/grpc++/impl/codegen/sync_stream.h index 4d9b074e95f..859c3d0b905 100644 --- a/include/grpc++/impl/codegen/sync_stream.h +++ b/include/grpc++/impl/codegen/sync_stream.h @@ -100,22 +100,40 @@ class WriterInterface { public: virtual ~WriterInterface() {} - /// Blocking write \a msg to the stream with options. + /// Blocking write \a msg to the stream with WriteOptions \a options. /// This is thread-safe with respect to \a Read /// /// \param msg The message to be written to the stream. - /// \param options Options affecting the write operation. + /// \param options The WriteOptions affecting the write operation. /// /// \return \a true on success, \a false when the stream has been closed. - virtual bool Write(const W& msg, const WriteOptions& options) = 0; + virtual bool Write(const W& msg, WriteOptions options) = 0; - /// Blocking write \a msg to the stream with default options. + /// Blocking write \a msg to the stream with default write options. /// This is thread-safe with respect to \a Read /// /// \param msg The message to be written to the stream. /// /// \return \a true on success, \a false when the stream has been closed. inline bool Write(const W& msg) { return Write(msg, WriteOptions()); } + + /// Write \a msg and coalesce it with the writing of trailing metadata, using + /// WriteOptions \a options. + /// + /// For client, WriteLast is equivalent of performing Write and WritesDone in + /// a single step. \a msg and trailing metadata are coalesced and sent on wire + /// by calling this function. + /// For server, WriteLast buffers the \a msg. The writing of \a msg is held + /// until the service handler returns, where \a msg and trailing metadata are + /// coalesced and sent on wire. Note that WriteLast can only buffer \a msg up + /// to the flow control window size. If \a msg size is larger than the window + /// size, it will be sent on wire without buffering. + /// + /// \param[in] msg The message to be written to the stream. + /// \param[in] options The WriteOptions to be used to write this message. + void WriteLast(const W& msg, WriteOptions options) { + Write(msg, options.set_last_message()); + } }; /// Client-side interface for streaming reads of message of type \a R. @@ -213,11 +231,13 @@ class ClientWriter : public ClientWriterInterface { finish_ops_.RecvMessage(response); finish_ops_.AllowNoMessage(); - CallOpSet ops; - ops.SendInitialMetadata(context->send_initial_metadata_, - context->initial_metadata_flags()); - call_.PerformOps(&ops); - cq_.Pluck(&ops); + if (!context_->initial_metadata_corked_) { + CallOpSet ops; + ops.SendInitialMetadata(context->send_initial_metadata_, + context->initial_metadata_flags()); + call_.PerformOps(&ops); + cq_.Pluck(&ops); + } } void WaitForInitialMetadata() { @@ -230,11 +250,24 @@ class ClientWriter : public ClientWriterInterface { } using WriterInterface::Write; - bool Write(const W& msg, const WriteOptions& options) override { - CallOpSet ops; + bool Write(const W& msg, WriteOptions options) override { + CallOpSet + ops; + + if (options.is_last_message()) { + options.set_buffer_hint(); + ops.ClientSendClose(); + } + if (context_->initial_metadata_corked_) { + ops.SendInitialMetadata(context_->send_initial_metadata_, + context_->initial_metadata_flags()); + context_->sent_initial_metadata_corked(false); + } if (!ops.SendMessage(msg, options).ok()) { return false; } + call_.PerformOps(&ops); return cq_.Pluck(&ops); } @@ -293,11 +326,13 @@ class ClientReaderWriter final : public ClientReaderWriterInterface { ClientReaderWriter(ChannelInterface* channel, const RpcMethod& method, ClientContext* context) : context_(context), call_(channel->CreateCall(method, context, &cq_)) { - CallOpSet ops; - ops.SendInitialMetadata(context->send_initial_metadata_, - context->initial_metadata_flags()); - call_.PerformOps(&ops); - cq_.Pluck(&ops); + if (!context_->initial_metadata_corked_) { + CallOpSet ops; + ops.SendInitialMetadata(context->send_initial_metadata_, + context->initial_metadata_flags()); + call_.PerformOps(&ops); + cq_.Pluck(&ops); + } } void WaitForInitialMetadata() override { @@ -325,9 +360,24 @@ class ClientReaderWriter final : public ClientReaderWriterInterface { } using WriterInterface::Write; - bool Write(const W& msg, const WriteOptions& options) override { - CallOpSet ops; - if (!ops.SendMessage(msg, options).ok()) return false; + bool Write(const W& msg, WriteOptions options) override { + CallOpSet + ops; + + if (options.is_last_message()) { + options.set_buffer_hint(); + ops.ClientSendClose(); + } + if (context_->initial_metadata_corked_) { + ops.SendInitialMetadata(context_->send_initial_metadata_, + context_->initial_metadata_flags()); + context_->sent_initial_metadata_corked(false); + } + if (!ops.SendMessage(msg, options).ok()) { + return false; + } + call_.PerformOps(&ops); return cq_.Pluck(&ops); } @@ -423,7 +473,10 @@ class ServerWriter final : public ServerWriterInterface { } using WriterInterface::Write; - bool Write(const W& msg, const WriteOptions& options) override { + bool Write(const W& msg, WriteOptions options) override { + if (options.is_last_message()) { + options.set_buffer_hint(); + } CallOpSet ops; if (!ops.SendMessage(msg, options).ok()) { return false; @@ -485,7 +538,10 @@ class ServerReaderWriterBody final { return call_->cq()->Pluck(&ops) && ops.got_message; } - bool Write(const W& msg, const WriteOptions& options) { + bool Write(const W& msg, WriteOptions options) { + if (options.is_last_message()) { + options.set_buffer_hint(); + } CallOpSet ops; if (!ops.SendMessage(msg, options).ok()) { return false; @@ -523,7 +579,7 @@ class ServerReaderWriter final : public ServerReaderWriterInterface { bool Read(R* msg) override { return body_.Read(msg); } using WriterInterface::Write; - bool Write(const W& msg, const WriteOptions& options) override { + bool Write(const W& msg, WriteOptions options) override { return body_.Write(msg, options); } @@ -562,8 +618,7 @@ class ServerUnaryStreamer final } using WriterInterface::Write; - bool Write(const ResponseType& response, - const WriteOptions& options) override { + bool Write(const ResponseType& response, WriteOptions options) override { if (write_done_ || !read_done_) { return false; } @@ -604,8 +659,7 @@ class ServerSplitStreamer final } using WriterInterface::Write; - bool Write(const ResponseType& response, - const WriteOptions& options) override { + bool Write(const ResponseType& response, WriteOptions options) override { return read_done_ && body_.Write(response, options); } diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index e5c731304cd..8e6f62a6fe5 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -298,8 +298,12 @@ typedef enum grpc_call_error { /** Force compression to be disabled for a particular write (start_write/add_metadata). Illegal on invoke/accept. */ #define GRPC_WRITE_NO_COMPRESS (0x00000002u) +/** Force coalescing of last streaming message and trailing metadata into the + same core batch */ +#define GRPC_WRITE_LAST_MESSAGE (0x00000004u) /** Mask of all valid flags. */ -#define GRPC_WRITE_USED_MASK (GRPC_WRITE_BUFFER_HINT | GRPC_WRITE_NO_COMPRESS) +#define GRPC_WRITE_USED_MASK \ + (GRPC_WRITE_BUFFER_HINT | GRPC_WRITE_NO_COMPRESS | GRPC_WRITE_LAST_MESSAGE) /* Initial metadata flags */ /** Signal that the call is idempotent */ @@ -311,13 +315,16 @@ typedef enum grpc_call_error { /** Signal that GRPC_INITIAL_METADATA_WAIT_FOR_READY was explicitly set by the calling application. */ #define GRPC_INITIAL_METADATA_WAIT_FOR_READY_EXPLICITLY_SET (0x00000080u) +/** Signal that the initial metadata should be corked */ +#define GRPC_INITIAL_METADATA_CORKED (0x00000100u) /** Mask of all valid flags */ -#define GRPC_INITIAL_METADATA_USED_MASK \ - (GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST | \ - GRPC_INITIAL_METADATA_WAIT_FOR_READY | \ - GRPC_INITIAL_METADATA_CACHEABLE_REQUEST | \ - GRPC_INITIAL_METADATA_WAIT_FOR_READY_EXPLICITLY_SET) +#define GRPC_INITIAL_METADATA_USED_MASK \ + (GRPC_INITIAL_METADATA_IDEMPOTENT_REQUEST | \ + GRPC_INITIAL_METADATA_WAIT_FOR_READY | \ + GRPC_INITIAL_METADATA_CACHEABLE_REQUEST | \ + GRPC_INITIAL_METADATA_WAIT_FOR_READY_EXPLICITLY_SET | \ + GRPC_INITIAL_METADATA_CORKED) /** A single metadata element */ typedef struct grpc_metadata { diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index da4c7dc7b23..2917fc5caa1 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -501,10 +501,11 @@ static void destroy_transport_locked(grpc_exec_ctx *exec_ctx, void *tp, static void destroy_transport(grpc_exec_ctx *exec_ctx, grpc_transport *gt) { grpc_chttp2_transport *t = (grpc_chttp2_transport *)gt; - grpc_closure_sched(exec_ctx, grpc_closure_create( - destroy_transport_locked, t, - grpc_combiner_scheduler(t->combiner, false)), - GRPC_ERROR_NONE); + grpc_closure_sched( + exec_ctx, + grpc_closure_create(destroy_transport_locked, t, + grpc_combiner_scheduler(t->combiner, false)), + GRPC_ERROR_NONE); } static void close_transport_locked(grpc_exec_ctx *exec_ctx, @@ -676,8 +677,9 @@ static void destroy_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, s->destroy_stream_arg = and_free_memory; grpc_closure_sched( - exec_ctx, grpc_closure_init(&s->destroy_stream, destroy_stream_locked, s, - grpc_combiner_scheduler(t->combiner, false)), + exec_ctx, + grpc_closure_init(&s->destroy_stream, destroy_stream_locked, s, + grpc_combiner_scheduler(t->combiner, false)), GRPC_ERROR_NONE); GPR_TIMER_END("destroy_stream", 0); } @@ -1206,8 +1208,13 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, void *stream_op, } } else { GPR_ASSERT(s->id != 0); - grpc_chttp2_become_writable(exec_ctx, t, s, - GRPC_CHTTP2_STREAM_WRITE_INITIATE_COVERED, + grpc_chttp2_stream_write_type write_type = + GRPC_CHTTP2_STREAM_WRITE_INITIATE_COVERED; + if (op->send_message != NULL && + (op->send_message->flags & GRPC_WRITE_BUFFER_HINT)) { + write_type = GRPC_CHTTP2_STREAM_WRITE_PIGGYBACK; + } + grpc_chttp2_become_writable(exec_ctx, t, s, write_type, "op.send_initial_metadata"); } } else { @@ -1470,9 +1477,10 @@ static void perform_transport_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, op->transport_private.args[0] = gt; GRPC_CHTTP2_REF_TRANSPORT(t, "transport_op"); grpc_closure_sched( - exec_ctx, grpc_closure_init(&op->transport_private.closure, - perform_transport_op_locked, op, - grpc_combiner_scheduler(t->combiner, false)), + exec_ctx, + grpc_closure_init(&op->transport_private.closure, + perform_transport_op_locked, op, + grpc_combiner_scheduler(t->combiner, false)), GRPC_ERROR_NONE); } @@ -2457,9 +2465,10 @@ static void destructive_reclaimer_locked(grpc_exec_ctx *exec_ctx, void *arg, s->id); } grpc_chttp2_cancel_stream( - exec_ctx, t, s, grpc_error_set_int(GRPC_ERROR_CREATE("Buffers full"), - GRPC_ERROR_INT_HTTP2_ERROR, - GRPC_HTTP2_ENHANCE_YOUR_CALM)); + exec_ctx, t, s, + grpc_error_set_int(GRPC_ERROR_CREATE("Buffers full"), + GRPC_ERROR_INT_HTTP2_ERROR, + GRPC_HTTP2_ENHANCE_YOUR_CALM)); if (n > 1) { /* Since we cancel one stream per destructive reclamation, if there are more streams left, we can immediately post a new diff --git a/src/cpp/client/client_context.cc b/src/cpp/client/client_context.cc index c073741dac0..96f97021cb7 100644 --- a/src/cpp/client/client_context.cc +++ b/src/cpp/client/client_context.cc @@ -67,7 +67,8 @@ ClientContext::ClientContext() call_canceled_(false), deadline_(gpr_inf_future(GPR_CLOCK_REALTIME)), census_context_(nullptr), - propagate_from_call_(nullptr) { + propagate_from_call_(nullptr), + initial_metadata_corked_(false) { g_client_callbacks->DefaultConstructor(this); } @@ -118,6 +119,10 @@ void ClientContext::set_compression_algorithm( AddMetadata(GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY, algorithm_name); } +void ClientContext::sent_initial_metadata_corked(bool corked) { + initial_metadata_corked_ = corked; +} + void ClientContext::TryCancel() { std::unique_lock lock(mu_); if (call_) { diff --git a/test/cpp/end2end/async_end2end_test.cc b/test/cpp/end2end/async_end2end_test.cc index 32e8a417958..47e803b948e 100644 --- a/test/cpp/end2end/async_end2end_test.cc +++ b/test/cpp/end2end/async_end2end_test.cc @@ -484,6 +484,81 @@ TEST_P(AsyncEnd2endTest, SimpleClientStreaming) { EXPECT_TRUE(recv_status.ok()); } +// Two pings and a final pong. +TEST_P(AsyncEnd2endTest, SimpleClientStreamingWithCoalescingApi) { + ResetStub(); + + EchoRequest send_request; + EchoRequest recv_request; + EchoResponse send_response; + EchoResponse recv_response; + Status recv_status; + ClientContext cli_ctx; + ServerContext srv_ctx; + ServerAsyncReader srv_stream(&srv_ctx); + + send_request.set_message(GetParam().message_content); + cli_ctx.sent_initial_metadata_corked(true); + // tag:1 never comes up since no op is performed + std::unique_ptr> cli_stream( + stub_->AsyncRequestStream(&cli_ctx, &recv_response, cq_.get(), tag(1))); + + service_.RequestRequestStream(&srv_ctx, &srv_stream, cq_.get(), cq_.get(), + tag(2)); + + cli_stream->Write(send_request, tag(3)); + + // 65536(64KB) is the default flow control window size. Should change this + // number when default flow control window size changes. For the write of + // send_request larger than the flow control window size, tag:3 will not come + // up until server read is initiated. For write of send_request smaller than + // the flow control window size, the request can take the free ride with + // initial metadata due to coalescing, thus write tag:3 will come up here. + if (GetParam().message_content.length() < 65536) { + Verifier(GetParam().disable_blocking) + .Expect(2, true) + .Expect(3, true) + .Verify(cq_.get()); + } else { + Verifier(GetParam().disable_blocking).Expect(2, true).Verify(cq_.get()); + } + + srv_stream.Read(&recv_request, tag(4)); + + if (GetParam().message_content.length() < 65536) { + Verifier(GetParam().disable_blocking).Expect(4, true).Verify(cq_.get()); + } else { + Verifier(GetParam().disable_blocking) + .Expect(3, true) + .Expect(4, true) + .Verify(cq_.get()); + } + + EXPECT_EQ(send_request.message(), recv_request.message()); + + cli_stream->WriteLast(send_request, WriteOptions(), tag(5)); + srv_stream.Read(&recv_request, tag(6)); + Verifier(GetParam().disable_blocking) + .Expect(5, true) + .Expect(6, true) + .Verify(cq_.get()); + EXPECT_EQ(send_request.message(), recv_request.message()); + + srv_stream.Read(&recv_request, tag(7)); + Verifier(GetParam().disable_blocking).Expect(7, false).Verify(cq_.get()); + + send_response.set_message(recv_request.message()); + srv_stream.Finish(send_response, Status::OK, tag(8)); + cli_stream->Finish(&recv_status, tag(9)); + Verifier(GetParam().disable_blocking) + .Expect(8, true) + .Expect(9, true) + .Verify(cq_.get()); + + EXPECT_EQ(send_response.message(), recv_response.message()); + EXPECT_TRUE(recv_status.ok()); +} + // One ping, two pongs. TEST_P(AsyncEnd2endTest, SimpleServerStreaming) { ResetStub(); @@ -540,6 +615,112 @@ TEST_P(AsyncEnd2endTest, SimpleServerStreaming) { EXPECT_TRUE(recv_status.ok()); } +// One ping, two pongs. Using WriteAndFinish API +TEST_P(AsyncEnd2endTest, SimpleServerStreamingWithCoalescingApiWAF) { + ResetStub(); + + EchoRequest send_request; + EchoRequest recv_request; + EchoResponse send_response; + EchoResponse recv_response; + Status recv_status; + ClientContext cli_ctx; + ServerContext srv_ctx; + ServerAsyncWriter srv_stream(&srv_ctx); + + send_request.set_message(GetParam().message_content); + std::unique_ptr> cli_stream( + stub_->AsyncResponseStream(&cli_ctx, send_request, cq_.get(), tag(1))); + + service_.RequestResponseStream(&srv_ctx, &recv_request, &srv_stream, + cq_.get(), cq_.get(), tag(2)); + + Verifier(GetParam().disable_blocking) + .Expect(1, true) + .Expect(2, true) + .Verify(cq_.get()); + EXPECT_EQ(send_request.message(), recv_request.message()); + + send_response.set_message(recv_request.message()); + srv_stream.Write(send_response, tag(3)); + cli_stream->Read(&recv_response, tag(4)); + Verifier(GetParam().disable_blocking) + .Expect(3, true) + .Expect(4, true) + .Verify(cq_.get()); + EXPECT_EQ(send_response.message(), recv_response.message()); + + srv_stream.WriteAndFinish(send_response, WriteOptions(), Status::OK, tag(5)); + cli_stream->Read(&recv_response, tag(6)); + Verifier(GetParam().disable_blocking) + .Expect(5, true) + .Expect(6, true) + .Verify(cq_.get()); + EXPECT_EQ(send_response.message(), recv_response.message()); + + cli_stream->Read(&recv_response, tag(7)); + Verifier(GetParam().disable_blocking).Expect(7, false).Verify(cq_.get()); + + cli_stream->Finish(&recv_status, tag(8)); + Verifier(GetParam().disable_blocking).Expect(8, true).Verify(cq_.get()); + + EXPECT_TRUE(recv_status.ok()); +} + +// One ping, two pongs. Using WriteLast API +TEST_P(AsyncEnd2endTest, SimpleServerStreamingWithCoalescingApiWL) { + ResetStub(); + + EchoRequest send_request; + EchoRequest recv_request; + EchoResponse send_response; + EchoResponse recv_response; + Status recv_status; + ClientContext cli_ctx; + ServerContext srv_ctx; + ServerAsyncWriter srv_stream(&srv_ctx); + + send_request.set_message(GetParam().message_content); + std::unique_ptr> cli_stream( + stub_->AsyncResponseStream(&cli_ctx, send_request, cq_.get(), tag(1))); + + service_.RequestResponseStream(&srv_ctx, &recv_request, &srv_stream, + cq_.get(), cq_.get(), tag(2)); + + Verifier(GetParam().disable_blocking) + .Expect(1, true) + .Expect(2, true) + .Verify(cq_.get()); + EXPECT_EQ(send_request.message(), recv_request.message()); + + send_response.set_message(recv_request.message()); + srv_stream.Write(send_response, tag(3)); + cli_stream->Read(&recv_response, tag(4)); + Verifier(GetParam().disable_blocking) + .Expect(3, true) + .Expect(4, true) + .Verify(cq_.get()); + EXPECT_EQ(send_response.message(), recv_response.message()); + + srv_stream.WriteLast(send_response, WriteOptions(), tag(5)); + cli_stream->Read(&recv_response, tag(6)); + srv_stream.Finish(Status::OK, tag(7)); + Verifier(GetParam().disable_blocking) + .Expect(5, true) + .Expect(6, true) + .Expect(7, true) + .Verify(cq_.get()); + EXPECT_EQ(send_response.message(), recv_response.message()); + + cli_stream->Read(&recv_response, tag(8)); + Verifier(GetParam().disable_blocking).Expect(8, false).Verify(cq_.get()); + + cli_stream->Finish(&recv_status, tag(9)); + Verifier(GetParam().disable_blocking).Expect(9, true).Verify(cq_.get()); + + EXPECT_TRUE(recv_status.ok()); +} + // One ping, one pong. TEST_P(AsyncEnd2endTest, SimpleBidiStreaming) { ResetStub(); @@ -599,6 +780,144 @@ TEST_P(AsyncEnd2endTest, SimpleBidiStreaming) { EXPECT_TRUE(recv_status.ok()); } +// One ping, one pong. Using server:WriteAndFinish api +TEST_P(AsyncEnd2endTest, SimpleBidiStreamingWithCoalescingApiWAF) { + ResetStub(); + + EchoRequest send_request; + EchoRequest recv_request; + EchoResponse send_response; + EchoResponse recv_response; + Status recv_status; + ClientContext cli_ctx; + ServerContext srv_ctx; + ServerAsyncReaderWriter srv_stream(&srv_ctx); + + send_request.set_message(GetParam().message_content); + cli_ctx.sent_initial_metadata_corked(true); + std::unique_ptr> + cli_stream(stub_->AsyncBidiStream(&cli_ctx, cq_.get(), tag(1))); + + service_.RequestBidiStream(&srv_ctx, &srv_stream, cq_.get(), cq_.get(), + tag(2)); + + cli_stream->WriteLast(send_request, WriteOptions(), tag(3)); + + // 65536(64KB) is the default flow control window size. Should change this + // number when default flow control window size changes. For the write of + // send_request larger than the flow control window size, tag:3 will not come + // up until server read is initiated. For write of send_request smaller than + // the flow control window size, the request can take the free ride with + // initial metadata due to coalescing, thus write tag:3 will come up here. + if (GetParam().message_content.length() < 65536) { + Verifier(GetParam().disable_blocking) + .Expect(2, true) + .Expect(3, true) + .Verify(cq_.get()); + } else { + Verifier(GetParam().disable_blocking).Expect(2, true).Verify(cq_.get()); + } + + srv_stream.Read(&recv_request, tag(4)); + + if (GetParam().message_content.length() < 65536) { + Verifier(GetParam().disable_blocking).Expect(4, true).Verify(cq_.get()); + } else { + Verifier(GetParam().disable_blocking) + .Expect(3, true) + .Expect(4, true) + .Verify(cq_.get()); + } + EXPECT_EQ(send_request.message(), recv_request.message()); + + srv_stream.Read(&recv_request, tag(5)); + Verifier(GetParam().disable_blocking).Expect(5, false).Verify(cq_.get()); + + send_response.set_message(recv_request.message()); + srv_stream.WriteAndFinish(send_response, WriteOptions(), Status::OK, tag(6)); + cli_stream->Read(&recv_response, tag(7)); + Verifier(GetParam().disable_blocking) + .Expect(6, true) + .Expect(7, true) + .Verify(cq_.get()); + EXPECT_EQ(send_response.message(), recv_response.message()); + + cli_stream->Finish(&recv_status, tag(8)); + Verifier(GetParam().disable_blocking).Expect(8, true).Verify(cq_.get()); + + EXPECT_TRUE(recv_status.ok()); +} + +// One ping, one pong. Using server:WriteLast api +TEST_P(AsyncEnd2endTest, SimpleBidiStreamingWithCoalescingApiWL) { + ResetStub(); + + EchoRequest send_request; + EchoRequest recv_request; + EchoResponse send_response; + EchoResponse recv_response; + Status recv_status; + ClientContext cli_ctx; + ServerContext srv_ctx; + ServerAsyncReaderWriter srv_stream(&srv_ctx); + + send_request.set_message(GetParam().message_content); + cli_ctx.sent_initial_metadata_corked(true); + std::unique_ptr> + cli_stream(stub_->AsyncBidiStream(&cli_ctx, cq_.get(), tag(1))); + + service_.RequestBidiStream(&srv_ctx, &srv_stream, cq_.get(), cq_.get(), + tag(2)); + + cli_stream->WriteLast(send_request, WriteOptions(), tag(3)); + + // 65536(64KB) is the default flow control window size. Should change this + // number when default flow control window size changes. For the write of + // send_request larger than the flow control window size, tag:3 will not come + // up until server read is initiated. For write of send_request smaller than + // the flow control window size, the request can take the free ride with + // initial metadata due to coalescing, thus write tag:3 will come up here. + if (GetParam().message_content.length() < 65536) { + Verifier(GetParam().disable_blocking) + .Expect(2, true) + .Expect(3, true) + .Verify(cq_.get()); + } else { + Verifier(GetParam().disable_blocking).Expect(2, true).Verify(cq_.get()); + } + + srv_stream.Read(&recv_request, tag(4)); + + if (GetParam().message_content.length() < 65536) { + Verifier(GetParam().disable_blocking).Expect(4, true).Verify(cq_.get()); + } else { + Verifier(GetParam().disable_blocking) + .Expect(3, true) + .Expect(4, true) + .Verify(cq_.get()); + } + EXPECT_EQ(send_request.message(), recv_request.message()); + + srv_stream.Read(&recv_request, tag(5)); + Verifier(GetParam().disable_blocking).Expect(5, false).Verify(cq_.get()); + + send_response.set_message(recv_request.message()); + srv_stream.WriteLast(send_response, WriteOptions(), tag(6)); + srv_stream.Finish(Status::OK, tag(7)); + cli_stream->Read(&recv_response, tag(8)); + Verifier(GetParam().disable_blocking) + .Expect(6, true) + .Expect(7, true) + .Expect(8, true) + .Verify(cq_.get()); + EXPECT_EQ(send_response.message(), recv_response.message()); + + cli_stream->Finish(&recv_status, tag(9)); + Verifier(GetParam().disable_blocking).Expect(9, true).Verify(cq_.get()); + + EXPECT_TRUE(recv_status.ok()); +} + // Metadata tests TEST_P(AsyncEnd2endTest, ClientInitialMetadataRpc) { ResetStub(); diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index df78557c437..222ab1ae836 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -702,6 +702,21 @@ TEST_P(End2endTest, RequestStreamOneRequest) { EXPECT_TRUE(s.ok()); } +TEST_P(End2endTest, RequestStreamOneRequestWithCoalescingApi) { + ResetStub(); + EchoRequest request; + EchoResponse response; + ClientContext context; + + context.sent_initial_metadata_corked(true); + auto stream = stub_->RequestStream(&context, &response); + request.set_message("hello"); + stream->WriteLast(request, WriteOptions()); + Status s = stream->Finish(); + EXPECT_EQ(response.message(), request.message()); + EXPECT_TRUE(s.ok()); +} + TEST_P(End2endTest, RequestStreamTwoRequests) { ResetStub(); EchoRequest request; @@ -718,6 +733,22 @@ TEST_P(End2endTest, RequestStreamTwoRequests) { EXPECT_TRUE(s.ok()); } +TEST_P(End2endTest, RequestStreamTwoRequestsWithCoalescingApi) { + ResetStub(); + EchoRequest request; + EchoResponse response; + ClientContext context; + + context.sent_initial_metadata_corked(true); + auto stream = stub_->RequestStream(&context, &response); + request.set_message("hello"); + EXPECT_TRUE(stream->Write(request)); + stream->WriteLast(request, WriteOptions()); + Status s = stream->Finish(); + EXPECT_EQ(response.message(), "hellohello"); + EXPECT_TRUE(s.ok()); +} + TEST_P(End2endTest, ResponseStream) { ResetStub(); EchoRequest request; @@ -738,6 +769,27 @@ TEST_P(End2endTest, ResponseStream) { EXPECT_TRUE(s.ok()); } +TEST_P(End2endTest, ResponseStreamWithCoalescingApi) { + ResetStub(); + EchoRequest request; + EchoResponse response; + ClientContext context; + request.set_message("hello"); + context.AddMetadata(kServerUseCoalescingApi, "1"); + + auto stream = stub_->ResponseStream(&context, request); + EXPECT_TRUE(stream->Read(&response)); + EXPECT_EQ(response.message(), request.message() + "0"); + EXPECT_TRUE(stream->Read(&response)); + EXPECT_EQ(response.message(), request.message() + "1"); + EXPECT_TRUE(stream->Read(&response)); + EXPECT_EQ(response.message(), request.message() + "2"); + EXPECT_FALSE(stream->Read(&response)); + + Status s = stream->Finish(); + EXPECT_TRUE(s.ok()); +} + TEST_P(End2endTest, BidiStream) { ResetStub(); EchoRequest request; @@ -770,6 +822,39 @@ TEST_P(End2endTest, BidiStream) { EXPECT_TRUE(s.ok()); } +TEST_P(End2endTest, BidiStreamWithCoalescingApi) { + ResetStub(); + EchoRequest request; + EchoResponse response; + ClientContext context; + context.AddMetadata(kServerFinishAfterNReads, "3"); + context.sent_initial_metadata_corked(true); + grpc::string msg("hello"); + + auto stream = stub_->BidiStream(&context); + + request.set_message(msg + "0"); + EXPECT_TRUE(stream->Write(request)); + EXPECT_TRUE(stream->Read(&response)); + EXPECT_EQ(response.message(), request.message()); + + request.set_message(msg + "1"); + EXPECT_TRUE(stream->Write(request)); + EXPECT_TRUE(stream->Read(&response)); + EXPECT_EQ(response.message(), request.message()); + + request.set_message(msg + "2"); + stream->WriteLast(request, WriteOptions()); + EXPECT_TRUE(stream->Read(&response)); + EXPECT_EQ(response.message(), request.message()); + + EXPECT_FALSE(stream->Read(&response)); + EXPECT_FALSE(stream->Read(&response)); + + Status s = stream->Finish(); + EXPECT_TRUE(s.ok()); +} + // Talk to the two services with the same name but different package names. // The two stubs are created on the same channel. TEST_P(End2endTest, DiffPackageServices) { diff --git a/test/cpp/end2end/test_service_impl.cc b/test/cpp/end2end/test_service_impl.cc index 59d36e9cb56..11729c425cd 100644 --- a/test/cpp/end2end/test_service_impl.cc +++ b/test/cpp/end2end/test_service_impl.cc @@ -246,6 +246,9 @@ Status TestServiceImpl::ResponseStream(ServerContext* context, int server_try_cancel = GetIntValueFromMetadata( kServerTryCancelRequest, context->client_metadata(), DO_NOT_CANCEL); + int server_coalescing_api = GetIntValueFromMetadata( + kServerUseCoalescingApi, context->client_metadata(), 0); + if (server_try_cancel == CANCEL_BEFORE_PROCESSING) { ServerTryCancel(context); return Status::CANCELLED; @@ -260,7 +263,11 @@ Status TestServiceImpl::ResponseStream(ServerContext* context, for (int i = 0; i < kNumResponseStreamsMsgs; i++) { response.set_message(request->message() + grpc::to_string(i)); - writer->Write(response); + if (i == kNumResponseStreamsMsgs - 1 && server_coalescing_api != 0) { + writer->WriteLast(response, WriteOptions()); + } else { + writer->Write(response); + } } if (server_try_cancel_thd != nullptr) { @@ -305,10 +312,21 @@ Status TestServiceImpl::BidiStream( new std::thread(&TestServiceImpl::ServerTryCancel, this, context); } + // kServerFinishAfterNReads suggests after how many reads, the server should + // write the last message and send status (coalesced using WriteLast) + int server_write_last = GetIntValueFromMetadata( + kServerFinishAfterNReads, context->client_metadata(), 0); + + int read_counts = 0; while (stream->Read(&request)) { + read_counts++; gpr_log(GPR_INFO, "recv msg %s", request.message().c_str()); response.set_message(request.message()); - stream->Write(response); + if (read_counts == server_write_last) { + stream->WriteLast(response, WriteOptions()); + } else { + stream->Write(response); + } } if (server_try_cancel_thd != nullptr) { diff --git a/test/cpp/end2end/test_service_impl.h b/test/cpp/end2end/test_service_impl.h index 88e0be7bca7..b1f02f93f65 100644 --- a/test/cpp/end2end/test_service_impl.h +++ b/test/cpp/end2end/test_service_impl.h @@ -48,6 +48,8 @@ const int kNumResponseStreamsMsgs = 3; const char* const kServerCancelAfterReads = "cancel_after_reads"; const char* const kServerTryCancelRequest = "server_try_cancel"; const char* const kDebugInfoTrailerKey = "debug-info-bin"; +const char* const kServerFinishAfterNReads = "server_finish_after_n_reads"; +const char* const kServerUseCoalescingApi = "server_use_coalescing_api"; typedef enum { DO_NOT_CANCEL = 0, diff --git a/test/cpp/microbenchmarks/bm_fullstack.cc b/test/cpp/microbenchmarks/bm_fullstack.cc index 48e131f1be0..77ea443f84d 100644 --- a/test/cpp/microbenchmarks/bm_fullstack.cc +++ b/test/cpp/microbenchmarks/bm_fullstack.cc @@ -111,9 +111,10 @@ class BaseFixture { std::ostringstream out; this->AddToLabel(out, s); #ifdef GPR_LOW_LEVEL_COUNTERS - out << " locks/iter:" << ((double)(gpr_atm_no_barrier_load(&gpr_mu_locks) - - mu_locks_at_start_) / - (double)s.iterations()) + out << " locks/iter:" + << ((double)(gpr_atm_no_barrier_load(&gpr_mu_locks) - + mu_locks_at_start_) / + (double)s.iterations()) << " atm_cas/iter:" << ((double)(gpr_atm_no_barrier_load(&gpr_counter_atm_cas) - atm_cas_at_start_) / @@ -754,6 +755,173 @@ static void BM_StreamingPingPongMsgs(benchmark::State& state) { state.SetBytesProcessed(msg_size * state.iterations() * 2); } +// Repeatedly makes Streaming Bidi calls (exchanging a configurable number of +// messages in each call) in a loop on a single channel. Different from +// BM_StreamingPingPong we are using stream coalescing api, e.g. WriteLast, +// WriteAndFinish, sent_initial_metadata_corked. These apis aim at saving +// sendmsg syscalls for streaming by coalescing 1. initial metadata with first +// message; 2. final streaming message with trailing metadata. +// +// First parmeter (i.e state.range(0)): Message size (in bytes) to use +// Second parameter (i.e state.range(1)): Number of ping pong messages. +// Note: One ping-pong means two messages (one from client to server and +// the other from server to client): +// Third parameter (i.e state.range(2)): Swtich between using WriteAndFinish +// API and WriteLast API for server. +template +static void BM_StreamingPingPongWithCoalescingApi(benchmark::State& state) { + const int msg_size = state.range(0); + const int max_ping_pongs = state.range(1); + // This options is used to test out server API: WriteLast and WriteAndFinish + // respectively, since we can not use both of them on server side at the same + // time. Value 1 means we are testing out the WriteAndFinish API, and + // otherwise we are testing out the WriteLast API. + const int write_and_finish = state.range(2); + + EchoTestService::AsyncService service; + std::unique_ptr fixture(new Fixture(&service)); + { + EchoResponse send_response; + EchoResponse recv_response; + EchoRequest send_request; + EchoRequest recv_request; + + if (msg_size > 0) { + send_request.set_message(std::string(msg_size, 'a')); + send_response.set_message(std::string(msg_size, 'b')); + } + + std::unique_ptr stub( + EchoTestService::NewStub(fixture->channel())); + + while (state.KeepRunning()) { + ServerContext svr_ctx; + ServerContextMutator svr_ctx_mut(&svr_ctx); + ServerAsyncReaderWriter response_rw(&svr_ctx); + service.RequestBidiStream(&svr_ctx, &response_rw, fixture->cq(), + fixture->cq(), tag(0)); + + ClientContext cli_ctx; + ClientContextMutator cli_ctx_mut(&cli_ctx); + cli_ctx.sent_initial_metadata_corked(true); + // tag:1 here will never comes up, since we are not performing any op due + // to initial metadata coalescing. + auto request_rw = stub->AsyncBidiStream(&cli_ctx, fixture->cq(), tag(1)); + + void* t; + bool ok; + int need_tags; + + // Send 'max_ping_pongs' number of ping pong messages + int ping_pong_cnt = 0; + while (ping_pong_cnt < max_ping_pongs) { + if (ping_pong_cnt == max_ping_pongs - 1) { + request_rw->WriteLast(send_request, WriteOptions(), tag(2)); + } else { + request_rw->Write(send_request, tag(2)); // Start client send + } + + need_tags = (1 << 2) | (1 << 3) | (1 << 4) | (1 << 5); + + if (ping_pong_cnt == 0) { + // wait for the server call structure (call_hook, etc.) to be + // initialized (async stream between client side and server side + // established). It is necessary when client init metadata is + // coalesced + GPR_ASSERT(fixture->cq()->Next(&t, &ok)); + while ((int)(intptr_t)t != 0) { + // In some cases tag:2 comes before tag:0 (write tag comes out + // first), this while loop is to make sure get tag:0. + int i = (int)(intptr_t)t; + GPR_ASSERT(need_tags & (1 << i)); + need_tags &= ~(1 << i); + GPR_ASSERT(fixture->cq()->Next(&t, &ok)); + } + } + + response_rw.Read(&recv_request, tag(3)); // Start server recv + request_rw->Read(&recv_response, tag(4)); // Start client recv + + while (need_tags) { + GPR_ASSERT(fixture->cq()->Next(&t, &ok)); + GPR_ASSERT(ok); + int i = (int)(intptr_t)t; + + // If server recv is complete, start the server send operation + if (i == 3) { + if (ping_pong_cnt == max_ping_pongs - 1) { + if (write_and_finish == 1) { + response_rw.WriteAndFinish(send_response, WriteOptions(), + Status::OK, tag(5)); + } else { + response_rw.WriteLast(send_response, WriteOptions(), tag(5)); + // WriteLast buffers the write, so neither server write op nor + // client read op will finish inside the while loop. + need_tags &= ~(1 << 4); + need_tags &= ~(1 << 5); + } + } else { + response_rw.Write(send_response, tag(5)); + } + } + + GPR_ASSERT(need_tags & (1 << i)); + need_tags &= ~(1 << i); + } + + ping_pong_cnt++; + } + + if (max_ping_pongs == 0) { + need_tags = (1 << 6) | (1 << 7) | (1 << 8); + } else { + if (write_and_finish == 1) { + need_tags = (1 << 8); + } else { + // server's buffered write and the client's read of the buffered write + // tags should come up. + need_tags = (1 << 4) | (1 << 5) | (1 << 7) | (1 << 8); + } + } + + // No message write or initial metadata write happened yet. + if (max_ping_pongs == 0) { + request_rw->WritesDone(tag(6)); + // wait for server call data structure(call_hook, etc.) to be + // initialized, since initial metadata is corked. + GPR_ASSERT(fixture->cq()->Next(&t, &ok)); + while ((int)(intptr_t)t != 0) { + int i = (int)(intptr_t)t; + GPR_ASSERT(need_tags & (1 << i)); + need_tags &= ~(1 << i); + GPR_ASSERT(fixture->cq()->Next(&t, &ok)); + } + response_rw.Finish(Status::OK, tag(7)); + } else { + if (write_and_finish != 1) { + response_rw.Finish(Status::OK, tag(7)); + } + } + + Status recv_status; + request_rw->Finish(&recv_status, tag(8)); + + while (need_tags) { + GPR_ASSERT(fixture->cq()->Next(&t, &ok)); + int i = (int)(intptr_t)t; + GPR_ASSERT(need_tags & (1 << i)); + need_tags &= ~(1 << i); + } + + GPR_ASSERT(recv_status.ok()); + } + } + + fixture->Finish(state); + fixture.reset(); + state.SetBytesProcessed(msg_size * state.iterations() * max_ping_pongs * 2); +} + template static void BM_PumpStreamClientToServer(benchmark::State& state) { EchoTestService::AsyncService service; @@ -873,8 +1041,9 @@ static void BM_PumpStreamServerToClient(benchmark::State& state) { static void TrickleCQNext(TrickledCHTTP2* fixture, void** t, bool* ok) { while (true) { switch (fixture->cq()->AsyncNext( - t, ok, gpr_time_add(gpr_now(GPR_CLOCK_MONOTONIC), - gpr_time_from_micros(100, GPR_TIMESPAN)))) { + t, ok, + gpr_time_add(gpr_now(GPR_CLOCK_MONOTONIC), + gpr_time_from_micros(100, GPR_TIMESPAN)))) { case CompletionQueue::TIMEOUT: fixture->Step(); break; @@ -1067,6 +1236,30 @@ BENCHMARK_TEMPLATE(BM_StreamingPingPong, InProcessCHTTP2, NoOpMutator, BENCHMARK_TEMPLATE(BM_StreamingPingPong, TCP, NoOpMutator, NoOpMutator) ->Apply(StreamingPingPongArgs); +// Generate Args for StreamingPingPongWithCoalescingApi benchmarks. Currently +// generates args for only "small streams" (i.e streams with 0, 1 or 2 messages) +static void StreamingPingPongWithCoalescingApiArgs( + benchmark::internal::Benchmark* b) { + int msg_size = 0; + + b->Args( + {0, 0, 0}); // spl case: 0 ping-pong msgs (msg_size doesn't matter here) + b->Args( + {0, 0, 1}); // spl case: 0 ping-pong msgs (msg_size doesn't matter here) + + for (msg_size = 0; msg_size <= 128 * 1024 * 1024; + msg_size == 0 ? msg_size++ : msg_size *= 8) { + b->Args({msg_size, 1, 0}); + b->Args({msg_size, 2, 0}); + b->Args({msg_size, 1, 1}); + b->Args({msg_size, 2, 1}); + } +} + +BENCHMARK_TEMPLATE(BM_StreamingPingPongWithCoalescingApi, InProcessCHTTP2, + NoOpMutator, NoOpMutator) + ->Apply(StreamingPingPongWithCoalescingApiArgs); + BENCHMARK_TEMPLATE(BM_StreamingPingPongMsgs, InProcessCHTTP2, NoOpMutator, NoOpMutator) ->Range(0, 128 * 1024 * 1024); From bdc76ab37b4d61a849f2f9f9742a5004864ce984 Mon Sep 17 00:00:00 2001 From: Yuxuan Li Date: Fri, 3 Mar 2017 13:59:30 -0800 Subject: [PATCH 16/26] bug fix --- include/grpc++/impl/codegen/async_stream.h | 4 ++-- include/grpc++/impl/codegen/client_context.h | 4 +++- include/grpc++/impl/codegen/sync_stream.h | 4 ++-- src/cpp/client/client_context.cc | 4 ---- test/cpp/end2end/async_end2end_test.cc | 6 +++--- test/cpp/end2end/end2end_test.cc | 6 +++--- test/cpp/end2end/mock_test.cc | 2 +- test/cpp/microbenchmarks/bm_fullstack.cc | 4 ++-- third_party/protobuf | 2 +- 9 files changed, 17 insertions(+), 19 deletions(-) diff --git a/include/grpc++/impl/codegen/async_stream.h b/include/grpc++/impl/codegen/async_stream.h index f21139618f5..a2013415902 100644 --- a/include/grpc++/impl/codegen/async_stream.h +++ b/include/grpc++/impl/codegen/async_stream.h @@ -244,7 +244,7 @@ class ClientAsyncWriter final : public ClientAsyncWriterInterface { call_.PerformOps(&write_ops_); } - void Write(const W& msg, WriteOptions options, void* tag) { + void Write(const W& msg, WriteOptions options, void* tag) override { write_ops_.set_output_tag(tag); if (options.is_last_message()) { options.set_buffer_hint(); @@ -341,7 +341,7 @@ class ClientAsyncReaderWriter final call_.PerformOps(&write_ops_); } - void Write(const W& msg, WriteOptions options, void* tag) { + void Write(const W& msg, WriteOptions options, void* tag) override { write_ops_.set_output_tag(tag); if (options.is_last_message()) { options.set_buffer_hint(); diff --git a/include/grpc++/impl/codegen/client_context.h b/include/grpc++/impl/codegen/client_context.h index cc48b9c3f57..3c50e6ba9d4 100644 --- a/include/grpc++/impl/codegen/client_context.h +++ b/include/grpc++/impl/codegen/client_context.h @@ -288,7 +288,9 @@ class ClientContext { /// /// \param corked The flag indicating whether the initial metadata is to be /// corked or not. - void sent_initial_metadata_corked(bool corked); + void set_initial_metadata_corked(bool corked) { + initial_metadata_corked_ = corked; + } /// Return the peer uri in a string. /// diff --git a/include/grpc++/impl/codegen/sync_stream.h b/include/grpc++/impl/codegen/sync_stream.h index 859c3d0b905..ae3b8e441d1 100644 --- a/include/grpc++/impl/codegen/sync_stream.h +++ b/include/grpc++/impl/codegen/sync_stream.h @@ -262,7 +262,7 @@ class ClientWriter : public ClientWriterInterface { if (context_->initial_metadata_corked_) { ops.SendInitialMetadata(context_->send_initial_metadata_, context_->initial_metadata_flags()); - context_->sent_initial_metadata_corked(false); + context_->set_initial_metadata_corked(false); } if (!ops.SendMessage(msg, options).ok()) { return false; @@ -372,7 +372,7 @@ class ClientReaderWriter final : public ClientReaderWriterInterface { if (context_->initial_metadata_corked_) { ops.SendInitialMetadata(context_->send_initial_metadata_, context_->initial_metadata_flags()); - context_->sent_initial_metadata_corked(false); + context_->set_initial_metadata_corked(false); } if (!ops.SendMessage(msg, options).ok()) { return false; diff --git a/src/cpp/client/client_context.cc b/src/cpp/client/client_context.cc index 96f97021cb7..3d884cf62e4 100644 --- a/src/cpp/client/client_context.cc +++ b/src/cpp/client/client_context.cc @@ -119,10 +119,6 @@ void ClientContext::set_compression_algorithm( AddMetadata(GRPC_COMPRESSION_REQUEST_ALGORITHM_MD_KEY, algorithm_name); } -void ClientContext::sent_initial_metadata_corked(bool corked) { - initial_metadata_corked_ = corked; -} - void ClientContext::TryCancel() { std::unique_lock lock(mu_); if (call_) { diff --git a/test/cpp/end2end/async_end2end_test.cc b/test/cpp/end2end/async_end2end_test.cc index 47e803b948e..0b5215ef8e4 100644 --- a/test/cpp/end2end/async_end2end_test.cc +++ b/test/cpp/end2end/async_end2end_test.cc @@ -498,7 +498,7 @@ TEST_P(AsyncEnd2endTest, SimpleClientStreamingWithCoalescingApi) { ServerAsyncReader srv_stream(&srv_ctx); send_request.set_message(GetParam().message_content); - cli_ctx.sent_initial_metadata_corked(true); + cli_ctx.set_initial_metadata_corked(true); // tag:1 never comes up since no op is performed std::unique_ptr> cli_stream( stub_->AsyncRequestStream(&cli_ctx, &recv_response, cq_.get(), tag(1))); @@ -794,7 +794,7 @@ TEST_P(AsyncEnd2endTest, SimpleBidiStreamingWithCoalescingApiWAF) { ServerAsyncReaderWriter srv_stream(&srv_ctx); send_request.set_message(GetParam().message_content); - cli_ctx.sent_initial_metadata_corked(true); + cli_ctx.set_initial_metadata_corked(true); std::unique_ptr> cli_stream(stub_->AsyncBidiStream(&cli_ctx, cq_.get(), tag(1))); @@ -862,7 +862,7 @@ TEST_P(AsyncEnd2endTest, SimpleBidiStreamingWithCoalescingApiWL) { ServerAsyncReaderWriter srv_stream(&srv_ctx); send_request.set_message(GetParam().message_content); - cli_ctx.sent_initial_metadata_corked(true); + cli_ctx.set_initial_metadata_corked(true); std::unique_ptr> cli_stream(stub_->AsyncBidiStream(&cli_ctx, cq_.get(), tag(1))); diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index 222ab1ae836..d3a83b188f9 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -708,7 +708,7 @@ TEST_P(End2endTest, RequestStreamOneRequestWithCoalescingApi) { EchoResponse response; ClientContext context; - context.sent_initial_metadata_corked(true); + context.set_initial_metadata_corked(true); auto stream = stub_->RequestStream(&context, &response); request.set_message("hello"); stream->WriteLast(request, WriteOptions()); @@ -739,7 +739,7 @@ TEST_P(End2endTest, RequestStreamTwoRequestsWithCoalescingApi) { EchoResponse response; ClientContext context; - context.sent_initial_metadata_corked(true); + context.set_initial_metadata_corked(true); auto stream = stub_->RequestStream(&context, &response); request.set_message("hello"); EXPECT_TRUE(stream->Write(request)); @@ -828,7 +828,7 @@ TEST_P(End2endTest, BidiStreamWithCoalescingApi) { EchoResponse response; ClientContext context; context.AddMetadata(kServerFinishAfterNReads, "3"); - context.sent_initial_metadata_corked(true); + context.set_initial_metadata_corked(true); grpc::string msg("hello"); auto stream = stub_->BidiStream(&context); diff --git a/test/cpp/end2end/mock_test.cc b/test/cpp/end2end/mock_test.cc index d6664da5a0f..fdb2732e503 100644 --- a/test/cpp/end2end/mock_test.cc +++ b/test/cpp/end2end/mock_test.cc @@ -89,7 +89,7 @@ class MockClientReaderWriter final return true; } - bool Write(const EchoRequest& msg, const WriteOptions& options) override { + bool Write(const EchoRequest& msg, WriteOptions options) override { gpr_log(GPR_INFO, "mock recv msg %s", msg.message().c_str()); last_message_ = msg.message(); return true; diff --git a/test/cpp/microbenchmarks/bm_fullstack.cc b/test/cpp/microbenchmarks/bm_fullstack.cc index 77ea443f84d..b1f3ed3597c 100644 --- a/test/cpp/microbenchmarks/bm_fullstack.cc +++ b/test/cpp/microbenchmarks/bm_fullstack.cc @@ -758,7 +758,7 @@ static void BM_StreamingPingPongMsgs(benchmark::State& state) { // Repeatedly makes Streaming Bidi calls (exchanging a configurable number of // messages in each call) in a loop on a single channel. Different from // BM_StreamingPingPong we are using stream coalescing api, e.g. WriteLast, -// WriteAndFinish, sent_initial_metadata_corked. These apis aim at saving +// WriteAndFinish, set_initial_metadata_corked. These apis aim at saving // sendmsg syscalls for streaming by coalescing 1. initial metadata with first // message; 2. final streaming message with trailing metadata. // @@ -803,7 +803,7 @@ static void BM_StreamingPingPongWithCoalescingApi(benchmark::State& state) { ClientContext cli_ctx; ClientContextMutator cli_ctx_mut(&cli_ctx); - cli_ctx.sent_initial_metadata_corked(true); + cli_ctx.set_initial_metadata_corked(true); // tag:1 here will never comes up, since we are not performing any op due // to initial metadata coalescing. auto request_rw = stub->AsyncBidiStream(&cli_ctx, fixture->cq(), tag(1)); diff --git a/third_party/protobuf b/third_party/protobuf index 593e917c176..a428e420727 160000 --- a/third_party/protobuf +++ b/third_party/protobuf @@ -1 +1 @@ -Subproject commit 593e917c176b5bc5aafa57bf9f6030d749d91cd5 +Subproject commit a428e42072765993ff674fda72863c9f1aa2d268 From 8124f66993d3c2eebb46832300d133a50e735f0f Mon Sep 17 00:00:00 2001 From: Yuxuan Li Date: Wed, 8 Mar 2017 11:26:02 -0800 Subject: [PATCH 17/26] protobuf correct head --- third_party/protobuf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/protobuf b/third_party/protobuf index a428e420727..593e917c176 160000 --- a/third_party/protobuf +++ b/third_party/protobuf @@ -1 +1 @@ -Subproject commit a428e42072765993ff674fda72863c9f1aa2d268 +Subproject commit 593e917c176b5bc5aafa57bf9f6030d749d91cd5 From 0aac9d06e24358c71472eca5bce4c51b499bb2cb Mon Sep 17 00:00:00 2001 From: Yuxuan Li Date: Fri, 10 Mar 2017 14:34:59 -0800 Subject: [PATCH 18/26] change last_message bit to be in C++ layer only --- include/grpc++/impl/codegen/call.h | 16 +++++++--------- include/grpc/impl/codegen/grpc_types.h | 6 +----- test/cpp/microbenchmarks/bm_fullstack.cc | 2 +- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index f6638471d24..a3f2be6bb1e 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -84,8 +84,9 @@ inline grpc_metadata* FillMetadataArray( /// Per-message write options. class WriteOptions { public: - WriteOptions() : flags_(0) {} - WriteOptions(const WriteOptions& other) : flags_(other.flags_) {} + WriteOptions() : flags_(0), last_message_(false) {} + WriteOptions(const WriteOptions& other) + : flags_(other.flags_), last_message_(other.last_message_) {} /// Clear all flags. inline void Clear() { flags_ = 0; } @@ -160,19 +161,15 @@ class WriteOptions { /// in a single step /// server-side: hold the Write until the service handler returns (sync api) /// or until Finish is called (async api) - /// - /// \sa GRPC_WRITE_LAST_MESSAGE inline WriteOptions& set_last_message() { - SetBit(GRPC_WRITE_LAST_MESSAGE); + last_message_ = true; return *this; } /// Clears flag indicating that this is the last message in a stream, /// disabling coalescing. - /// - /// \sa GRPC_WRITE_LAST_MESSAGE inline WriteOptions& clear_last_messsage() { - ClearBit(GRPC_WRITE_LAST_MESSAGE); + last_message_ = false; return *this; } @@ -180,7 +177,7 @@ class WriteOptions { /// should be coalesced with trailing metadata. /// /// \sa GRPC_WRITE_LAST_MESSAGE - bool is_last_message() const { return GetBit(GRPC_WRITE_LAST_MESSAGE); } + bool is_last_message() const { return last_message_; } WriteOptions& operator=(const WriteOptions& rhs) { flags_ = rhs.flags_; @@ -195,6 +192,7 @@ class WriteOptions { bool GetBit(const uint32_t mask) const { return (flags_ & mask) != 0; } uint32_t flags_; + bool last_message_; }; /// Default argument for CallOpSet. I is unused by the class, but can be diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 8e6f62a6fe5..a076b8c619b 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -298,12 +298,8 @@ typedef enum grpc_call_error { /** Force compression to be disabled for a particular write (start_write/add_metadata). Illegal on invoke/accept. */ #define GRPC_WRITE_NO_COMPRESS (0x00000002u) -/** Force coalescing of last streaming message and trailing metadata into the - same core batch */ -#define GRPC_WRITE_LAST_MESSAGE (0x00000004u) /** Mask of all valid flags. */ -#define GRPC_WRITE_USED_MASK \ - (GRPC_WRITE_BUFFER_HINT | GRPC_WRITE_NO_COMPRESS | GRPC_WRITE_LAST_MESSAGE) +#define GRPC_WRITE_USED_MASK (GRPC_WRITE_BUFFER_HINT | GRPC_WRITE_NO_COMPRESS) /* Initial metadata flags */ /** Signal that the call is idempotent */ diff --git a/test/cpp/microbenchmarks/bm_fullstack.cc b/test/cpp/microbenchmarks/bm_fullstack.cc index b1f3ed3597c..b5b68c9d095 100644 --- a/test/cpp/microbenchmarks/bm_fullstack.cc +++ b/test/cpp/microbenchmarks/bm_fullstack.cc @@ -766,7 +766,7 @@ static void BM_StreamingPingPongMsgs(benchmark::State& state) { // Second parameter (i.e state.range(1)): Number of ping pong messages. // Note: One ping-pong means two messages (one from client to server and // the other from server to client): -// Third parameter (i.e state.range(2)): Swtich between using WriteAndFinish +// Third parameter (i.e state.range(2)): Switch between using WriteAndFinish // API and WriteLast API for server. template static void BM_StreamingPingPongWithCoalescingApi(benchmark::State& state) { From 44e1837ef914b6ad4133a3eaf38761ca37d9ae83 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Fri, 10 Mar 2017 14:52:51 -0800 Subject: [PATCH 19/26] Add comment for ignoring errorHandler in GRPCOpSendMessage and TODO --- src/objective-c/GRPCClient/GRPCCall.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index ff474650122..7d928f81c18 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -307,6 +307,8 @@ static NSMutableDictionary *callFlags; [_wrappedCall startBatchWithOperations:@[op] errorHandler:errorHandler]; } else { + // Ignored errorHandler since it is the same as the one for GRPCOpSendClose. + // TODO (mxyan): unify the error handlers of all Ops into a single closure. [_unaryOpBatch addObject:op]; } } From 01bbf87bf78d40c9a8287fccf7ea0d64ff5cf07c Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Fri, 10 Mar 2017 15:02:28 -0800 Subject: [PATCH 20/26] Add comments for map function --- src/objective-c/RxLibrary/GRXImmediateSingleWriter.m | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m index a0d3b771e8d..a3e9cd60a35 100644 --- a/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m +++ b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m @@ -75,7 +75,11 @@ return; } +// Overrides [requestWriter(Transformations):map:] for Protocol Buffers +// encoding. - (GRXWriter *)map:(id (^)(id))map { + // Since _value is available when creating the object, we can simply + // apply the map and store the output. _value = map(_value); return self; } From 7e151845ab580a7fd5fb72922d3381034045d7b4 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Fri, 10 Mar 2017 16:22:19 -0800 Subject: [PATCH 21/26] Wrap entire GRPCCall (InternalTests) class in GRPC_TEST_OBJC --- .../internal_testing/GRPCCall+InternalTests.h | 4 +++ .../internal_testing/GRPCCall+InternalTests.m | 17 +++------- src/objective-c/tests/Podfile | 4 +-- .../tests/Tests.xcodeproj/project.pbxproj | 32 +++++++++++++++++-- 4 files changed, 39 insertions(+), 18 deletions(-) diff --git a/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.h b/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.h index 6c12ce6355e..29bd12f0cfb 100644 --- a/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.h +++ b/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.h @@ -31,6 +31,8 @@ * */ +#ifdef GRPC_TEST_OBJC + #import "../GRPCCall.h" /** @@ -55,3 +57,5 @@ + (NSArray *)obtainAndCleanOpBatchLog; @end + +#endif diff --git a/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.m b/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.m index 43c1c078316..6371df6739a 100644 --- a/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.m +++ b/src/objective-c/GRPCClient/internal_testing/GRPCCall+InternalTests.m @@ -31,6 +31,8 @@ * */ +#ifdef GRPC_TEST_OBJC + #import "GRPCCall+InternalTests.h" #import "../private/GRPCOpBatchLog.h" @@ -38,24 +40,13 @@ @implementation GRPCCall (InternalTests) + (void)enableOpBatchLog:(BOOL)enabled { -#ifdef GRPC_TEST_OBJC [GRPCOpBatchLog enableOpBatchLog:enabled]; -#else - NSLog(@"This function is for internal testing of gRPC only. " - "It is not part of gRPC's public interface. Do not use in production. " - "To enable, set the preprocessor flag GRPC_TEST_OBJC."); -#endif } + (NSArray *)obtainAndCleanOpBatchLog { -#ifdef GRPC_TEST_OBJC return [GRPCOpBatchLog obtainAndCleanOpBatchLog]; -#else - NSLog(@"This function is for internal testing of gRPC only. " - "It is not part of gRPC's public interface. Do not use in production. " - "To enable, set the preprocessor flag GRPC_TEST_OBJC."); - return nil; -#endif } @end + +#endif diff --git a/src/objective-c/tests/Podfile b/src/objective-c/tests/Podfile index 941031ff88e..8f1cb041d86 100644 --- a/src/objective-c/tests/Podfile +++ b/src/objective-c/tests/Podfile @@ -108,9 +108,9 @@ post_install do |installer| if target.name == 'gRPC' target.build_configurations.each do |config| if config.name == 'Cronet' - config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_COMPILE_WITH_CRONET=1 GRPC_TEST_OBJC' + config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_COMPILE_WITH_CRONET=1 GRPC_TEST_OBJC=1' elsif config.name == 'Test' - config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_TEST_OBJC' + config.build_settings['GCC_PREPROCESSOR_DEFINITIONS'] = '$(inherited) COCOAPODS=1 GRPC_TEST_OBJC=1' end end end diff --git a/src/objective-c/tests/Tests.xcodeproj/project.pbxproj b/src/objective-c/tests/Tests.xcodeproj/project.pbxproj index 57e417c2a8a..97de723a22a 100644 --- a/src/objective-c/tests/Tests.xcodeproj/project.pbxproj +++ b/src/objective-c/tests/Tests.xcodeproj/project.pbxproj @@ -1319,6 +1319,7 @@ GCC_PREPROCESSOR_DEFINITIONS = ( "DEBUG=1", "$(inherited)", + "GRPC_TEST_OBJC=1", ); INFOPLIST_FILE = Info.plist; LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; @@ -1346,6 +1347,13 @@ buildSettings = { DEBUG_INFORMATION_FORMAT = dwarf; ENABLE_TESTABILITY = YES; + GCC_PREPROCESSOR_DEFINITIONS = ( + "$(inherited)", + "COCOAPODS=1", + "$(inherited)", + "GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS=1", + "GRPC_TEST_OBJC=1", + ); INFOPLIST_FILE = Info.plist; IPHONEOS_DEPLOYMENT_TARGET = 9.0; LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; @@ -1360,6 +1368,13 @@ buildSettings = { DEBUG_INFORMATION_FORMAT = dwarf; ENABLE_TESTABILITY = YES; + GCC_PREPROCESSOR_DEFINITIONS = ( + "$(inherited)", + "COCOAPODS=1", + "$(inherited)", + "GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS=1", + "GRPC_TEST_OBJC=1", + ); INFOPLIST_FILE = Info.plist; IPHONEOS_DEPLOYMENT_TARGET = 9.0; LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; @@ -1374,6 +1389,12 @@ buildSettings = { DEBUG_INFORMATION_FORMAT = dwarf; ENABLE_TESTABILITY = YES; + GCC_PREPROCESSOR_DEFINITIONS = ( + "$(inherited)", + "COCOAPODS=1", + "GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS=1", + "GRPC_TEST_OBJC=1", + ); INFOPLIST_FILE = Info.plist; IPHONEOS_DEPLOYMENT_TARGET = 9.0; LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; @@ -1410,8 +1431,8 @@ GCC_PREPROCESSOR_DEFINITIONS = ( "$(inherited)", "COCOAPODS=1", - "$(inherited)", "GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS=1", + "GRPC_TEST_OBJC=1", ); INFOPLIST_FILE = InteropTestsRemoteWithCronet/Info.plist; IPHONEOS_DEPLOYMENT_TARGET = 9.3; @@ -1608,6 +1629,12 @@ buildSettings = { DEBUG_INFORMATION_FORMAT = dwarf; ENABLE_TESTABILITY = YES; + GCC_PREPROCESSOR_DEFINITIONS = ( + "$(inherited)", + "COCOAPODS=1", + "$(inherited)", + "GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS=1", + ); INFOPLIST_FILE = Info.plist; IPHONEOS_DEPLOYMENT_TARGET = 9.0; LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks @loader_path/Frameworks"; @@ -1672,10 +1699,10 @@ GCC_PREPROCESSOR_DEFINITIONS = ( "$(inherited)", "COCOAPODS=1", - "$(inherited)", "GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS=1", "GRPC_COMPILE_WITH_CRONET=1", "GRPC_CRONET_WITH_PACKET_COALESCING=1", + "GRPC_TEST_OBJC=1", ); INFOPLIST_FILE = InteropTestsRemoteWithCronet/Info.plist; IPHONEOS_DEPLOYMENT_TARGET = 9.3; @@ -1696,7 +1723,6 @@ GCC_PREPROCESSOR_DEFINITIONS = ( "$(inherited)", "COCOAPODS=1", - "$(inherited)", "GPB_USE_PROTOBUF_FRAMEWORK_IMPORTS=1", ); INFOPLIST_FILE = InteropTestsRemoteWithCronet/Info.plist; From b39eeac79e8662922b38c9d72cad81a7fe47b714 Mon Sep 17 00:00:00 2001 From: Yuxuan Li Date: Mon, 20 Mar 2017 12:00:34 -0700 Subject: [PATCH 22/26] merge init_ops, writes_done_ops and write_ops --- include/grpc++/impl/codegen/async_stream.h | 118 +++++++-------------- 1 file changed, 40 insertions(+), 78 deletions(-) diff --git a/include/grpc++/impl/codegen/async_stream.h b/include/grpc++/impl/codegen/async_stream.h index a2013415902..f5fbb53c814 100644 --- a/include/grpc++/impl/codegen/async_stream.h +++ b/include/grpc++/impl/codegen/async_stream.h @@ -222,10 +222,10 @@ class ClientAsyncWriter final : public ClientAsyncWriterInterface { write_ops_.SendInitialMetadata(context->send_initial_metadata_, context->initial_metadata_flags()); } else { - init_ops_.set_output_tag(tag); - init_ops_.SendInitialMetadata(context->send_initial_metadata_, - context->initial_metadata_flags()); - call_.PerformOps(&init_ops_); + write_ops_.set_output_tag(tag); + write_ops_.SendInitialMetadata(context->send_initial_metadata_, + context->initial_metadata_flags()); + call_.PerformOps(&write_ops_); } } @@ -256,9 +256,9 @@ class ClientAsyncWriter final : public ClientAsyncWriterInterface { } void WritesDone(void* tag) override { - writes_done_ops_.set_output_tag(tag); - writes_done_ops_.ClientSendClose(); - call_.PerformOps(&writes_done_ops_); + write_ops_.set_output_tag(tag); + write_ops_.ClientSendClose(); + call_.PerformOps(&write_ops_); } void Finish(Status* status, void* tag) override { @@ -273,11 +273,9 @@ class ClientAsyncWriter final : public ClientAsyncWriterInterface { private: ClientContext* context_; Call call_; - CallOpSet init_ops_; CallOpSet meta_ops_; CallOpSet write_ops_; - CallOpSet writes_done_ops_; CallOpSet finish_ops_; @@ -310,10 +308,10 @@ class ClientAsyncReaderWriter final write_ops_.SendInitialMetadata(context->send_initial_metadata_, context->initial_metadata_flags()); } else { - init_ops_.set_output_tag(tag); - init_ops_.SendInitialMetadata(context->send_initial_metadata_, - context->initial_metadata_flags()); - call_.PerformOps(&init_ops_); + write_ops_.set_output_tag(tag); + write_ops_.SendInitialMetadata(context->send_initial_metadata_, + context->initial_metadata_flags()); + call_.PerformOps(&write_ops_); } } @@ -370,7 +368,6 @@ class ClientAsyncReaderWriter final private: ClientContext* context_; Call call_; - CallOpSet init_ops_; CallOpSet meta_ops_; CallOpSet> read_ops_; CallOpSet @@ -454,9 +451,7 @@ class ServerAsyncReader final : public ServerAsyncReaderInterface { ServerContext* ctx_; CallOpSet meta_ops_; CallOpSet> read_ops_; - CallOpSet - finish_ops_; + CallOpSet finish_ops_; }; template @@ -499,16 +494,20 @@ class ServerAsyncWriter final : public ServerAsyncWriterInterface { call_.PerformOps(&meta_ops_); } - void Write(const W& msg, void* tag) override { - write_ops_.set_output_tag(tag); + void EnsureInitialMetadataSent(CallOpSetInterface* ops) { if (!ctx_->sent_initial_metadata_) { - write_ops_.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); + ops.SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); if (ctx_->compression_level_set()) { - write_ops_.set_compression_level(ctx_->compression_level()); + ops.set_compression_level(ctx_->compression_level()); } ctx_->sent_initial_metadata_ = true; } + } + + void Write(const W& msg, void* tag) override { + write_ops_.set_output_tag(tag); + EnsureInitialMetadataSent(&write_ops_); // TODO(ctiller): don't assert GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg).ok()); call_.PerformOps(&write_ops_); @@ -520,14 +519,7 @@ class ServerAsyncWriter final : public ServerAsyncWriterInterface { options.set_buffer_hint(); } - if (!ctx_->sent_initial_metadata_) { - write_ops_.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); - if (ctx_->compression_level_set()) { - write_ops_.set_compression_level(ctx_->compression_level()); - } - ctx_->sent_initial_metadata_ = true; - } + EnsureInitialMetadataSent(&write_ops_); // TODO(ctiller): don't assert GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); call_.PerformOps(&write_ops_); @@ -536,14 +528,7 @@ class ServerAsyncWriter final : public ServerAsyncWriterInterface { void WriteAndFinish(const W& msg, WriteOptions options, const Status& status, void* tag) override { write_ops_.set_output_tag(tag); - if (!ctx_->sent_initial_metadata_) { - write_ops_.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); - if (ctx_->compression_level_set()) { - write_ops_.set_compression_level(ctx_->compression_level()); - } - ctx_->sent_initial_metadata_ = true; - } + EnsureInitialMetadataSent(&write_ops_); options.set_buffer_hint(); GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); write_ops_.ServerSendStatus(ctx_->trailing_metadata_, status); @@ -552,14 +537,7 @@ class ServerAsyncWriter final : public ServerAsyncWriterInterface { void Finish(const Status& status, void* tag) override { finish_ops_.set_output_tag(tag); - if (!ctx_->sent_initial_metadata_) { - finish_ops_.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); - if (ctx_->compression_level_set()) { - finish_ops_.set_compression_level(ctx_->compression_level()); - } - ctx_->sent_initial_metadata_ = true; - } + EnsureInitialMetadataSent(&finish_ops_); finish_ops_.ServerSendStatus(ctx_->trailing_metadata_, status); call_.PerformOps(&finish_ops_); } @@ -619,6 +597,17 @@ class ServerAsyncReaderWriter final call_.PerformOps(&meta_ops_); } + void EnsureInitialMetadataSent(CallOpSetInterface* ops) { + if (!ctx_->sent_initial_metadata_) { + ops.SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); + if (ctx_->compression_level_set()) { + ops.set_compression_level(ctx_->compression_level()); + } + ctx_->sent_initial_metadata_ = true; + } + } + void Read(R* msg, void* tag) override { read_ops_.set_output_tag(tag); read_ops_.RecvMessage(msg); @@ -627,14 +616,7 @@ class ServerAsyncReaderWriter final void Write(const W& msg, void* tag) override { write_ops_.set_output_tag(tag); - if (!ctx_->sent_initial_metadata_) { - write_ops_.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); - if (ctx_->compression_level_set()) { - write_ops_.set_compression_level(ctx_->compression_level()); - } - ctx_->sent_initial_metadata_ = true; - } + EnsureInitialMetadataSent(&write_ops_); // TODO(ctiller): don't assert GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg).ok()); call_.PerformOps(&write_ops_); @@ -645,14 +627,7 @@ class ServerAsyncReaderWriter final if (options.is_last_message()) { options.set_buffer_hint(); } - if (!ctx_->sent_initial_metadata_) { - write_ops_.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); - if (ctx_->compression_level_set()) { - write_ops_.set_compression_level(ctx_->compression_level()); - } - ctx_->sent_initial_metadata_ = true; - } + EnsureInitialMetadataSent(&write_ops_); GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); call_.PerformOps(&write_ops_); } @@ -660,14 +635,7 @@ class ServerAsyncReaderWriter final void WriteAndFinish(const W& msg, WriteOptions options, const Status& status, void* tag) override { write_ops_.set_output_tag(tag); - if (!ctx_->sent_initial_metadata_) { - write_ops_.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); - if (ctx_->compression_level_set()) { - write_ops_.set_compression_level(ctx_->compression_level()); - } - ctx_->sent_initial_metadata_ = true; - } + EnsureInitialMetadataSent(&write_ops_); options.set_buffer_hint(); GPR_CODEGEN_ASSERT(write_ops_.SendMessage(msg, options).ok()); write_ops_.ServerSendStatus(ctx_->trailing_metadata_, status); @@ -676,14 +644,8 @@ class ServerAsyncReaderWriter final void Finish(const Status& status, void* tag) override { finish_ops_.set_output_tag(tag); - if (!ctx_->sent_initial_metadata_) { - finish_ops_.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); - if (ctx_->compression_level_set()) { - finish_ops_.set_compression_level(ctx_->compression_level()); - } - ctx_->sent_initial_metadata_ = true; - } + EnsureInitialMetadataSent(&finish_ops_); + finish_ops_.ServerSendStatus(ctx_->trailing_metadata_, status); call_.PerformOps(&finish_ops_); } From 8d00f52d5b090585f4c66af0d17d359e5893ee5b Mon Sep 17 00:00:00 2001 From: Yuxuan Li Date: Mon, 20 Mar 2017 12:35:38 -0700 Subject: [PATCH 23/26] fix EnsureInitialMetadataSent type casting --- include/grpc++/impl/codegen/async_stream.h | 24 ++++++++++++++-------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/include/grpc++/impl/codegen/async_stream.h b/include/grpc++/impl/codegen/async_stream.h index f5fbb53c814..c67e5e22479 100644 --- a/include/grpc++/impl/codegen/async_stream.h +++ b/include/grpc++/impl/codegen/async_stream.h @@ -451,7 +451,9 @@ class ServerAsyncReader final : public ServerAsyncReaderInterface { ServerContext* ctx_; CallOpSet meta_ops_; CallOpSet> read_ops_; - CallOpSet finish_ops_; + CallOpSet + finish_ops_; }; template @@ -494,12 +496,14 @@ class ServerAsyncWriter final : public ServerAsyncWriterInterface { call_.PerformOps(&meta_ops_); } - void EnsureInitialMetadataSent(CallOpSetInterface* ops) { + void EnsureInitialMetadataSent(CallOpSetInterface* ops_in) { + CallOpSet* ops = + static_cast*>(ops_in); if (!ctx_->sent_initial_metadata_) { - ops.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); + ops->SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); if (ctx_->compression_level_set()) { - ops.set_compression_level(ctx_->compression_level()); + ops->set_compression_level(ctx_->compression_level()); } ctx_->sent_initial_metadata_ = true; } @@ -597,12 +601,14 @@ class ServerAsyncReaderWriter final call_.PerformOps(&meta_ops_); } - void EnsureInitialMetadataSent(CallOpSetInterface* ops) { + void EnsureInitialMetadataSent(CallOpSetInterface* ops_in) { + CallOpSet* ops = + static_cast*>(ops_in); if (!ctx_->sent_initial_metadata_) { - ops.SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); + ops->SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); if (ctx_->compression_level_set()) { - ops.set_compression_level(ctx_->compression_level()); + ops->set_compression_level(ctx_->compression_level()); } ctx_->sent_initial_metadata_ = true; } From c2e53b5af4830c14d5cda72bba20df2855789300 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Wed, 22 Mar 2017 14:30:16 -0700 Subject: [PATCH 24/26] Addressed the comments --- src/objective-c/GRPCClient/GRPCCall.m | 5 ++++- .../GRPCClient/private/GRPCOpBatchLog.h | 5 +++++ .../GRPCClient/private/GRPCOpBatchLog.m | 10 +++++++--- .../RxLibrary/GRXImmediateSingleWriter.m | 15 +++++++-------- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/objective-c/GRPCClient/GRPCCall.m b/src/objective-c/GRPCClient/GRPCCall.m index 7d928f81c18..051138ea4da 100644 --- a/src/objective-c/GRPCClient/GRPCCall.m +++ b/src/objective-c/GRPCClient/GRPCCall.m @@ -107,8 +107,11 @@ static NSMutableDictionary *callFlags; GRPCRequestHeaders *_requestHeaders; + // In the case that the call is a unary call (i.e. the writer to GRPCCall is of type + // GRXImmediateSingleWriter), GRPCCall will delay sending ops (not send them to C core + // immediately) and buffer them into a batch _unaryOpBatch. The batch is sent to C core when + // the SendClose op is added. BOOL _unaryCall; - NSMutableArray *_unaryOpBatch; } diff --git a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.h b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.h index dbc3417acf7..753c4cfee6f 100644 --- a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.h +++ b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.h @@ -31,6 +31,9 @@ * */ + +#ifdef GRPC_TEST_OBJC + /** * Logs the op batches of a client. Used for testing. */ @@ -52,3 +55,5 @@ + (NSArray *)obtainAndCleanOpBatchLog; @end + +#endif diff --git a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m index c172c3c988c..4b40baf122b 100644 --- a/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m +++ b/src/objective-c/GRPCClient/private/GRPCOpBatchLog.m @@ -31,11 +31,13 @@ * */ +#ifdef GRPC_TEST_OBJC + #import "GRPCOpBatchLog.h" -@implementation GRPCOpBatchLog +static NSMutableArray *opBatchLog = nil; -NSMutableArray *opBatchLog = nil; +@implementation GRPCOpBatchLog + (void)enableOpBatchLog:(BOOL)enabled { @synchronized (opBatchLog) { @@ -65,4 +67,6 @@ NSMutableArray *opBatchLog = nil; } } -@end \ No newline at end of file +@end + +#endif diff --git a/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m index a3e9cd60a35..a0da71cb769 100644 --- a/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m +++ b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m @@ -35,41 +35,40 @@ @implementation GRXImmediateSingleWriter { id _value; - NSError *_errorOrNil; id _writeable; } @synthesize state = _state; -- (instancetype)initWithValue:(id)value error:(NSError *)errorOrNil { +- (instancetype)initWithValue:(id)value { if (self = [super init]) { _value = value; - _errorOrNil = errorOrNil; _state = GRXWriterStateNotStarted; } return self; } + (GRXWriter *)writerWithValue:(id)value { - return [[self alloc] initWithValue:value error:nil]; + return [[self alloc] initWithValue:value]; } - (void)startWithWriteable:(id)writeable { _state = GRXWriterStateStarted; _writeable = writeable; [writeable writeValue:_value]; - [self finishWithError:_errorOrNil]; + [self finish]; } -- (void)finishWithError:(NSError *)errorOrNil { +- (void)finish { _state = GRXWriterStateFinished; - _errorOrNil = nil; _value = nil; id writeable = _writeable; _writeable = nil; - [writeable writesFinishedWithError:errorOrNil]; + [writeable writesFinishedWithError:nil]; } +// Overwrite the setter to disallow manual state transition. The getter +// of _state is synthesized. - (void)setState:(GRXWriterState)newState { // Manual state transition is not allowed return; From e5ddd4b906b99781709b5f77b9c2b277df9d51c4 Mon Sep 17 00:00:00 2001 From: Yuxuan Li Date: Mon, 27 Mar 2017 13:25:33 -0700 Subject: [PATCH 25/26] change EnsureInitialMetadataSent to use template --- include/grpc++/impl/codegen/async_stream.h | 50 +++++++++---------- .../chttp2/transport/chttp2_transport.c | 28 +++++------ 2 files changed, 36 insertions(+), 42 deletions(-) diff --git a/include/grpc++/impl/codegen/async_stream.h b/include/grpc++/impl/codegen/async_stream.h index c67e5e22479..8f529895cac 100644 --- a/include/grpc++/impl/codegen/async_stream.h +++ b/include/grpc++/impl/codegen/async_stream.h @@ -496,19 +496,6 @@ class ServerAsyncWriter final : public ServerAsyncWriterInterface { call_.PerformOps(&meta_ops_); } - void EnsureInitialMetadataSent(CallOpSetInterface* ops_in) { - CallOpSet* ops = - static_cast*>(ops_in); - if (!ctx_->sent_initial_metadata_) { - ops->SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); - if (ctx_->compression_level_set()) { - ops->set_compression_level(ctx_->compression_level()); - } - ctx_->sent_initial_metadata_ = true; - } - } - void Write(const W& msg, void* tag) override { write_ops_.set_output_tag(tag); EnsureInitialMetadataSent(&write_ops_); @@ -549,6 +536,18 @@ class ServerAsyncWriter final : public ServerAsyncWriterInterface { private: void BindCall(Call* call) override { call_ = *call; } + template + void EnsureInitialMetadataSent(T* ops) { + if (!ctx_->sent_initial_metadata_) { + ops->SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); + if (ctx_->compression_level_set()) { + ops->set_compression_level(ctx_->compression_level()); + } + ctx_->sent_initial_metadata_ = true; + } + } + Call call_; ServerContext* ctx_; CallOpSet meta_ops_; @@ -601,19 +600,6 @@ class ServerAsyncReaderWriter final call_.PerformOps(&meta_ops_); } - void EnsureInitialMetadataSent(CallOpSetInterface* ops_in) { - CallOpSet* ops = - static_cast*>(ops_in); - if (!ctx_->sent_initial_metadata_) { - ops->SendInitialMetadata(ctx_->initial_metadata_, - ctx_->initial_metadata_flags()); - if (ctx_->compression_level_set()) { - ops->set_compression_level(ctx_->compression_level()); - } - ctx_->sent_initial_metadata_ = true; - } - } - void Read(R* msg, void* tag) override { read_ops_.set_output_tag(tag); read_ops_.RecvMessage(msg); @@ -661,6 +647,18 @@ class ServerAsyncReaderWriter final void BindCall(Call* call) override { call_ = *call; } + template + void EnsureInitialMetadataSent(T* ops) { + if (!ctx_->sent_initial_metadata_) { + ops->SendInitialMetadata(ctx_->initial_metadata_, + ctx_->initial_metadata_flags()); + if (ctx_->compression_level_set()) { + ops->set_compression_level(ctx_->compression_level()); + } + ctx_->sent_initial_metadata_ = true; + } + } + Call call_; ServerContext* ctx_; CallOpSet meta_ops_; diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index a63d4e59548..7d80044cfee 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -501,11 +501,10 @@ static void destroy_transport_locked(grpc_exec_ctx *exec_ctx, void *tp, static void destroy_transport(grpc_exec_ctx *exec_ctx, grpc_transport *gt) { grpc_chttp2_transport *t = (grpc_chttp2_transport *)gt; - grpc_closure_sched( - exec_ctx, - grpc_closure_create(destroy_transport_locked, t, - grpc_combiner_scheduler(t->combiner, false)), - GRPC_ERROR_NONE); + grpc_closure_sched(exec_ctx, grpc_closure_create( + destroy_transport_locked, t, + grpc_combiner_scheduler(t->combiner, false)), + GRPC_ERROR_NONE); } static void close_transport_locked(grpc_exec_ctx *exec_ctx, @@ -678,9 +677,8 @@ static void destroy_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, s->destroy_stream_arg = then_schedule_closure; grpc_closure_sched( - exec_ctx, - grpc_closure_init(&s->destroy_stream, destroy_stream_locked, s, - grpc_combiner_scheduler(t->combiner, false)), + exec_ctx, grpc_closure_init(&s->destroy_stream, destroy_stream_locked, s, + grpc_combiner_scheduler(t->combiner, false)), GRPC_ERROR_NONE); GPR_TIMER_END("destroy_stream", 0); } @@ -1478,10 +1476,9 @@ static void perform_transport_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, op->transport_private.args[0] = gt; GRPC_CHTTP2_REF_TRANSPORT(t, "transport_op"); grpc_closure_sched( - exec_ctx, - grpc_closure_init(&op->transport_private.closure, - perform_transport_op_locked, op, - grpc_combiner_scheduler(t->combiner, false)), + exec_ctx, grpc_closure_init(&op->transport_private.closure, + perform_transport_op_locked, op, + grpc_combiner_scheduler(t->combiner, false)), GRPC_ERROR_NONE); } @@ -2470,10 +2467,9 @@ static void destructive_reclaimer_locked(grpc_exec_ctx *exec_ctx, void *arg, s->id); } grpc_chttp2_cancel_stream( - exec_ctx, t, s, - grpc_error_set_int(GRPC_ERROR_CREATE("Buffers full"), - GRPC_ERROR_INT_HTTP2_ERROR, - GRPC_HTTP2_ENHANCE_YOUR_CALM)); + exec_ctx, t, s, grpc_error_set_int(GRPC_ERROR_CREATE("Buffers full"), + GRPC_ERROR_INT_HTTP2_ERROR, + GRPC_HTTP2_ENHANCE_YOUR_CALM)); if (n > 1) { /* Since we cancel one stream per destructive reclamation, if there are more streams left, we can immediately post a new From 29a912df09c9d7edf5b6efdb4f45c9b7b428fc4d Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 27 Mar 2017 15:14:23 -0700 Subject: [PATCH 26/26] Add more comments --- src/objective-c/RxLibrary/GRXImmediateSingleWriter.m | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m index a0da71cb769..0096c996d4c 100644 --- a/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m +++ b/src/objective-c/RxLibrary/GRXImmediateSingleWriter.m @@ -76,6 +76,9 @@ // Overrides [requestWriter(Transformations):map:] for Protocol Buffers // encoding. +// We need the return value of this map to be a GRXImmediateSingleWriter but +// the original \a map function returns a new Writer of another type. So we +// need to override this function here. - (GRXWriter *)map:(id (^)(id))map { // Since _value is available when creating the object, we can simply // apply the map and store the output. @@ -83,4 +86,4 @@ return self; } -@end \ No newline at end of file +@end