Use 64kb read buffer in CRNHttpProtocolHandler on iOS.

components/cronet/ios/test/cronet_http_test.mm

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import <Cronet/Cronet.h>
 #import <Foundation/Foundation.h>
 
 #include <stdint.h>
 
 #include "base/logging.h"
@@ skipping 10 matching lines @@
 
 #include "url/gurl.h"
 
 @interface TestDelegate : NSObject<NSURLSessionDataDelegate,
                                    NSURLSessionDelegate,
                                    NSURLSessionTaskDelegate>
 
 // Completion semaphore for this TestDelegate. When the request this delegate is
 // attached to finishes (either successfully or with an error), this delegate
 // signals this semaphore.
-@property(assign, nonatomic) dispatch_semaphore_t semaphore;
-
-// Body of response received by the request this delegate is attached to.
-@property(retain, nonatomic) NSString* responseBody;
+@property(assign, atomic) dispatch_semaphore_t semaphore;
 
 // Error the request this delegate is attached to failed with, if any.
-@property(retain, nonatomic) NSError* error;
+@property(retain, atomic) NSError* error;
 
 @end
 
 @implementation TestDelegate
 @synthesize semaphore = _semaphore;
-@synthesize responseBody = _responseBody;
 @synthesize error = _error;
 
+NSMutableArray<NSData*>* _responseData;
+
 - (id)init {
   if (self = [super init]) {
     _semaphore = dispatch_semaphore_create(0);
   }
   return self;
 }
 
 - (void)dealloc {
   dispatch_release(_semaphore);
   [_error release];
   _error = nil;
   [super dealloc];
 }
 
 - (void)reset {
-  _responseBody = nil;
+  [_responseData dealloc];

[kapishnikov, 2017/04/19 16:14:16]

Are the test compiled under ARC? If yes, it should be enough to set
_responseData to nil.

[mef, 2017/04/19 17:39:09]

Currently they are NOT. IIUIC Chrome does not use ARC, but I'm not sure whether
Cronet or its tests should. Here is a patch of this CL with ARC enabled for
Cronet targets: https://codereview.chromium.org/2829703002

WDYT?

[kapishnikov, 2017/04/19 18:19:01]

I think ARC is the way to go.  Let's keep the enabling of ARC as a separate CL.

[mef, 2017/04/19 19:04:48]

ack, sgtm

+  _responseData = nil;
   _error = nil;
 }
 
+- (NSString*)responseBody {
+  if (_responseData == nil) {
+    return nil;
+  }
+  NSMutableString* body = [NSMutableString string];
+  for (NSData* data in _responseData) {
+    [body appendString:[[NSString alloc] initWithData:data
+                                             encoding:NSUTF8StringEncoding]];
+  }
+  VLOG(3) << "responseBody size:" << [body length]
+          << " chunks:" << [_responseData count];
+  return body;
+}
+
 - (void)URLSession:(NSURLSession*)session
     didBecomeInvalidWithError:(NSError*)error {
 }
 
 - (void)URLSession:(NSURLSession*)session
                     task:(NSURLSessionTask*)task
     didCompleteWithError:(NSError*)error {
   [self setError:error];
   dispatch_semaphore_signal(_semaphore);
 }
@@ skipping 11 matching lines @@
               dataTask:(NSURLSessionDataTask*)dataTask
     didReceiveResponse:(NSURLResponse*)response
      completionHandler:(void (^)(NSURLSessionResponseDisposition disposition))
                            completionHandler {
   completionHandler(NSURLSessionResponseAllow);
 }
 
 - (void)URLSession:(NSURLSession*)session
           dataTask:(NSURLSessionDataTask*)dataTask
     didReceiveData:(NSData*)data {
-  NSString* stringData =
-      [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding];
-  if (_responseBody == nil) {
-    _responseBody = stringData;
-  } else {
-    _responseBody = [_responseBody stringByAppendingString:stringData];
+  if (_responseData == nil) {
+    _responseData = [[NSMutableArray alloc] init];
   }
+  [_responseData addObject:data];
 }
 
 - (void)URLSession:(NSURLSession*)session
              dataTask:(NSURLSessionDataTask*)dataTask
     willCacheResponse:(NSCachedURLResponse*)proposedResponse
     completionHandler:
         (void (^)(NSCachedURLResponse* cachedResponse))completionHandler {
   completionHandler(proposedResponse);
 }
 
 @end
 
 namespace cronet {
-// base::TimeDelta would normally be ideal for this but it does not support
-// nanosecond resolution.
-static const int64_t ns_in_second = 1000000000LL;
 const char kUserAgent[] = "CronetTest/1.0.0.0";
 
 class HttpTest : public ::testing::Test {
  protected:
   HttpTest() {}
   ~HttpTest() override {}
 
   void SetUp() override {
     grpc_support::StartQuicTestServer();
     TestServer::Start();
@@ skipping 18 matching lines @@
   void TearDown() override {
     grpc_support::ShutdownQuicTestServer();
     TestServer::Shutdown();
   }
 
   // Launches the supplied |task| and blocks until it completes, with a timeout
   // of 1 second.
   void StartDataTaskAndWaitForCompletion(NSURLSessionDataTask* task) {
     [delegate_ reset];
     [task resume];
-    int64_t deadline_ns = 1 * ns_in_second;
-    dispatch_semaphore_wait([delegate_ semaphore],
-                            dispatch_time(DISPATCH_TIME_NOW, deadline_ns));
+    dispatch_semaphore_wait([delegate_ semaphore], DISPATCH_TIME_FOREVER);

[kapishnikov, 2017/04/19 16:14:16]

Why are we changing the timeout to DISPATCH_TIME_FOREVER? I think it would be
better to have some reasonable timeout and check the return value for 0.

[mef, 2017/04/19 17:39:09]

Done. One second was too short for perf tests. I've changed it to 20 seconds.

   }
 
   base::scoped_nsobject<NSURLSession> session_;
   base::scoped_nsobject<TestDelegate> delegate_;
 };
 
 TEST_F(HttpTest, NSURLSessionReceivesData) {
-  NSURL* url = net::NSURLWithGURL(GURL(grpc_support::kTestServerUrl));
+  NSURL* url = net::NSURLWithGURL(GURL(grpc_support::kTestServerSimpleUrl));
   __block BOOL block_used = NO;
   NSURLSessionDataTask* task = [session_ dataTaskWithURL:url];
   [Cronet setRequestFilterBlock:^(NSURLRequest* request) {
     block_used = YES;
     EXPECT_EQ([request URL], url);
     return YES;
   }];
   StartDataTaskAndWaitForCompletion(task);
   EXPECT_TRUE(block_used);
   EXPECT_EQ(nil, [delegate_ error]);
-  EXPECT_STREQ(grpc_support::kHelloBodyValue,
+  EXPECT_STREQ(grpc_support::kSimpleBodyValue,
                base::SysNSStringToUTF8([delegate_ responseBody]).c_str());
 }
 
+TEST_F(HttpTest, NSURLSessionReceivesBigHttpDataLoop) {
+  int iterations = 50;
+  long size = 10 * 1024 * 1024;
+  LOG(INFO) << "Downloading " << size << " bytes " << iterations << " times.";
+  NSTimeInterval elapsed_avg = 0;
+  NSTimeInterval elapsed_max = 0;
+  NSURL* url = net::NSURLWithGURL(GURL(TestServer::GetBigDataURL(size)));
+  for (int i = 0; i < iterations; ++i) {
+    [delegate_ reset];
+    __block BOOL block_used = NO;
+    NSURLSessionDataTask* task = [session_ dataTaskWithURL:url];
+    [Cronet setRequestFilterBlock:^(NSURLRequest* request) {
+      block_used = YES;
+      EXPECT_EQ([request URL], url);
+      return YES;
+    }];
+    NSDate* date = [NSDate date];
+    StartDataTaskAndWaitForCompletion(task);
+    NSTimeInterval elapsed = [date timeIntervalSinceNow] * -1000.0;

[kapishnikov, 2017/04/19 18:19:01]

NSTimeInterval should always contain the time interval in seconds (see
https://developer.apple.com/reference/foundation/nstimeinterval). Using it to
hold time in milleseconds could be misleading. I suggest, we convert the time to
milliseconds at the time of logging.

[mef, 2017/04/19 19:04:48]

Good point. Done.

+    elapsed_avg += elapsed;
+    if (elapsed > elapsed_max)
+      elapsed_max = elapsed;
+    EXPECT_TRUE(block_used);
+    EXPECT_EQ(nil, [delegate_ error]);
+  }
+  LOG(INFO) << "Elapsed Average:" << elapsed_avg / iterations
+            << "ms Max:" << elapsed_max << "ms";
+}
+
 TEST_F(HttpTest, GetGlobalMetricsDeltas) {
   NSData* delta1 = [Cronet getGlobalMetricsDeltas];
 
-  NSURL* url = net::NSURLWithGURL(GURL(grpc_support::kTestServerUrl));
+  NSURL* url = net::NSURLWithGURL(GURL(grpc_support::kTestServerSimpleUrl));
   NSURLSessionDataTask* task = [session_ dataTaskWithURL:url];
   StartDataTaskAndWaitForCompletion(task);
   EXPECT_EQ(nil, [delegate_ error]);
-  EXPECT_STREQ(grpc_support::kHelloBodyValue,
+  EXPECT_STREQ(grpc_support::kSimpleBodyValue,
                base::SysNSStringToUTF8([delegate_ responseBody]).c_str());
 
   NSData* delta2 = [Cronet getGlobalMetricsDeltas];
   EXPECT_FALSE([delta2 isEqualToData:delta1]);
 }
 
 TEST_F(HttpTest, SdchDisabledByDefault) {
   NSURL* url =
       net::NSURLWithGURL(GURL(TestServer::GetEchoHeaderURL("Accept-Encoding")));
   NSURLSessionDataTask* task = [session_ dataTaskWithURL:url];
@@ skipping 209 matching lines @@
   [Cronet setRequestFilterBlock:^(NSURLRequest* request) {
     EXPECT_TRUE(false) << "Block should not be called for unsupported requests";
     return YES;
   }];
   StartDataTaskAndWaitForCompletion(task);
   EXPECT_EQ(nil, [delegate_ error]);
   EXPECT_TRUE([[delegate_ responseBody] containsString:testString]);
 }
 
 }  // namespace cronet