Fixed bug in cronet_transport where completed ops were not removed fr…

…om storage
grpc · Oct 4, 2019 · 4bb7731 · 4bb7731
1 parent 04495e5
commit 4bb7731
Show file tree

Hide file tree

Showing 9 changed files with 56 additions and 22 deletions.
diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.cc b/src/core/ext/transport/cronet/transport/cronet_transport.cc
@@ -1348,21 +1348,26 @@ static enum e_op_result execute_stream_op(struct op_and_state* oas) {
       stream_state->cancel_error =
           GRPC_ERROR_REF(stream_op->payload->cancel_stream.cancel_error);
     }
-  } else if (stream_op->on_complete &&
-             op_can_be_run(stream_op, s, &oas->state, OP_ON_COMPLETE)) {
+  } else if (op_can_be_run(stream_op, s, &oas->state, OP_ON_COMPLETE)) {
     CRONET_LOG(GPR_DEBUG, "running: %p  OP_ON_COMPLETE", oas);
     if (stream_state->state_op_done[OP_CANCEL_ERROR]) {
-      GRPC_CLOSURE_SCHED(stream_op->on_complete,
-                         GRPC_ERROR_REF(stream_state->cancel_error));
+      if (stream_op->on_complete) {
+        GRPC_CLOSURE_SCHED(stream_op->on_complete,
+                           GRPC_ERROR_REF(stream_state->cancel_error));
+      }
     } else if (stream_state->state_callback_received[OP_FAILED]) {
-      GRPC_CLOSURE_SCHED(
-          stream_op->on_complete,
-          make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."));
+      if (stream_op->on_complete) {
+        GRPC_CLOSURE_SCHED(
+            stream_op->on_complete,
+            make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."));
+      }
     } else {
       /* All actions in this stream_op are complete. Call the on_complete
        * callback
        */
-      GRPC_CLOSURE_SCHED(stream_op->on_complete, GRPC_ERROR_NONE);
+      if (stream_op->on_complete) {
+        GRPC_CLOSURE_SCHED(stream_op->on_complete, GRPC_ERROR_NONE);
+      }
     }
     oas->state.state_op_done[OP_ON_COMPLETE] = true;
     oas->done = true;

diff --git a/src/objective-c/tests/ConfigureCronet.h b/src/objective-c/tests/ConfigureCronet.h
@@ -15,6 +15,7 @@
  * limitations under the License.
  *
  */
+#include <stdbool.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -23,7 +24,7 @@ extern "C" {
 /**
  * Enable Cronet for once.
  */
-void configureCronet(void);
+void configureCronet(bool enable_netlog);
 
 #ifdef __cplusplus
 }

diff --git a/src/objective-c/tests/ConfigureCronet.m b/src/objective-c/tests/ConfigureCronet.m
@@ -19,7 +19,7 @@
 #import "ConfigureCronet.h"
 #import <Cronet/Cronet.h>
 
-void configureCronet(void) {
+void configureCronet(bool enable_netlog) {
   static dispatch_once_t configureCronet;
   dispatch_once(&configureCronet, ^{
     NSLog(@"configureCronet()");
@@ -29,7 +29,9 @@ void configureCronet(void) {
     NSURL *url = [[[NSFileManager defaultManager] URLsForDirectory:NSDocumentDirectory
                                                          inDomains:NSUserDomainMask] lastObject];
     NSLog(@"Documents directory: %@", url);
+    if (enable_netlog) {
+      [Cronet startNetLogToFile:@"cronet_netlog.json" logBytes:YES];
+    }
     [Cronet start];
-    [Cronet startNetLogToFile:@"cronet_netlog.json" logBytes:YES];
   });
 }
diff --git a/src/objective-c/tests/CronetTests/CoreCronetEnd2EndTests.mm b/src/objective-c/tests/CronetTests/CoreCronetEnd2EndTests.mm
@@ -177,7 +177,7 @@ + (void)setUp {
 
   grpc_init();
 
-  configureCronet();
+  configureCronet(/*enable_netlog=*/false);
 }
 
 // The tearDown() function is run after all test cases finish running

diff --git a/src/objective-c/tests/CronetTests/CronetUnitTests.mm b/src/objective-c/tests/CronetTests/CronetUnitTests.mm
@@ -64,7 +64,7 @@ + (void)setUp {
   grpc_test_init(1, argv);
 
   grpc_init();
-  configureCronet();
+  configureCronet(/*enable_netlog=*/false);
   init_ssl();
 }
 

diff --git a/src/objective-c/tests/CronetTests/InteropTestsRemoteWithCronet.m b/src/objective-c/tests/CronetTests/InteropTestsRemoteWithCronet.m
@@ -42,7 +42,7 @@ @interface InteropTestsRemoteWithCronet : InteropTests
 @implementation InteropTestsRemoteWithCronet
 
 + (void)setUp {
-  configureCronet();
+  configureCronet(/*enable_netlog=*/false);
   [GRPCCall useCronetWithEngine:[Cronet getGlobalEngine]];
 
   [super setUp];

diff --git a/src/objective-c/tests/InteropTests/InteropTestsMultipleChannels.m b/src/objective-c/tests/InteropTests/InteropTestsMultipleChannels.m
@@ -85,7 +85,7 @@ - (void)setUp {
   self.continueAfterFailure = NO;
 
   _remoteService = [RMTTestService serviceWithHost:kRemoteSSLHost callOptions:nil];
-  configureCronet();
+  configureCronet(/*enable_netlog=*/false);
 
   // Default stack with remote host
   GRPCMutableCallOptions *options = [[GRPCMutableCallOptions alloc] init];

diff --git a/src/objective-c/tests/PerfTests/PerfTests.m b/src/objective-c/tests/PerfTests/PerfTests.m
@@ -143,8 +143,8 @@ - (void)pingPongV2APIWithRequest:(RMTStreamingOutputCallRequest *)request
                                             messageCallback:^(id message) {
                                               int indexCopy;
                                               @synchronized(self) {
-                                                index += 1;
                                                 indexCopy = index;
+                                                index += 1;
                                               }
                                               if (indexCopy < numMessages) {
                                                 [call writeMessage:request];
@@ -181,7 +181,7 @@ - (void)testPingPongRPCWithV2API {
   [self pingPongV2APIWithRequest:request numMessages:1000 options:options];
 
   [self measureBlock:^{
-    [self pingPongV2APIWithRequest:request numMessages:10000 options:options];
+    [self pingPongV2APIWithRequest:request numMessages:1000 options:options];
   }];
 }
 
@@ -198,7 +198,7 @@ - (void)testPingPongRPCWithFlowControl {
   [self pingPongV2APIWithRequest:request numMessages:1000 options:options];
 
   [self measureBlock:^{
-    [self pingPongV2APIWithRequest:request numMessages:10000 options:options];
+    [self pingPongV2APIWithRequest:request numMessages:1000 options:options];
   }];
 }
 
@@ -215,7 +215,7 @@ - (void)testPingPongRPCWithInterceptor {
   [self pingPongV2APIWithRequest:request numMessages:1000 options:options];
 
   [self measureBlock:^{
-    [self pingPongV2APIWithRequest:request numMessages:10000 options:options];
+    [self pingPongV2APIWithRequest:request numMessages:1000 options:options];
   }];
 }
 
@@ -254,7 +254,7 @@ - (void)testPingPongRPCWithV1API {
   id request = [RMTStreamingOutputCallRequest messageWithPayloadSize:@1 requestedResponseSize:@1];
   [self pingPongV1APIWithRequest:request numMessages:1000];
   [self measureBlock:^{
-    [self pingPongV1APIWithRequest:request numMessages:10000];
+    [self pingPongV1APIWithRequest:request numMessages:1000];
   }];
 }
 
@@ -298,7 +298,7 @@ - (void)unaryRPCsWithServices:(NSArray<RMTTestService *> *)services
   [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil];
 }
 
-- (void)testUnaryRPC {
+- (void)test1MBUnaryRPC {
   // Workaround Apple CFStream bug
   if ([self isUsingCFStream]) {
     return;
@@ -329,6 +329,32 @@ - (void)testUnaryRPC {
   }];
 }
 
+- (void)test1KBUnaryRPC {
+  RMTSimpleRequest *request = [RMTSimpleRequest message];
+  request.responseSize = 1024;
+  request.payload.body = [NSMutableData dataWithLength:1024];
+
+  GRPCMutableCallOptions *options = [[GRPCMutableCallOptions alloc] init];
+  options.transport = [[self class] transport];
+  options.PEMRootCertificates = [[self class] PEMRootCertificates];
+  options.hostNameOverride = [[self class] hostNameOverride];
+
+  // warm up
+  [self unaryRPCsWithServices:@[ self->_service ]
+                      request:request
+              callsPerService:1000
+          maxOutstandingCalls:100
+                  callOptions:options];
+
+  [self measureBlock:^{
+    [self unaryRPCsWithServices:@[ self->_service ]
+                        request:request
+                callsPerService:1000
+            maxOutstandingCalls:100
+                    callOptions:options];
+  }];
+}
+
 - (void)testMultipleChannels {
   NSString *port = [[[self class] host] componentsSeparatedByString:@":"][1];
   int kNumAddrs = 10;

diff --git a/src/objective-c/tests/PerfTests/PerfTestsCronet.m b/src/objective-c/tests/PerfTests/PerfTestsCronet.m
@@ -42,7 +42,7 @@ @interface PerfTestsCronet : PerfTests
 @implementation PerfTestsCronet
 
 + (void)setUp {
-  configureCronet();
+  configureCronet(/*enable_netlog=*/false);
   [GRPCCall useCronetWithEngine:[Cronet getGlobalEngine]];
 
   [super setUp];