Fix: remediate a crash in santametricservice (#729)

* Fix issue with task cancelation.

* Make export timeouts configurable.

This allows an export timeout to be set via configuration and eases testing.

Source/common/SNTConfigurator.h

@@ -438,6 +438,11 @@
 ///
 @property(readonly, nonatomic) NSUInteger metricExportInterval;
 
+///
+/// Duration in seconds for metrics export timeout. Defaults to 30;
+///
+@property(readonly, nonatomic) NSUInteger metricExportTimeout;
+
 ///
 ///  Retrieve an initialized singleton configurator object using the default file path.
 ///

Source/common/SNTConfigurator.m

@@ -110,6 +110,7 @@ @implementation SNTConfigurator
 static NSString *const kMetricFormat = @"MetricFormat";
 static NSString *const kMetricURL = @"MetricURL";
 static NSString *const kMetricExportInterval = @"MetricExportInterval";
+static NSString *const kMetricExportTimeout = @"MetricExportTimeout";
 static NSString *const kMetricExtraLabels = @"MetricExtraLabels";
 
 // The keys managed by a sync server.
@@ -193,6 +194,7 @@ - (instancetype)init {
       kMetricFormat : string,
       kMetricURL : string,
       kMetricExportInterval : number,
+      kMetricExportTimeout : number,
       kMetricExtraLabels : dictionary,
     };
     _defaults = [NSUserDefaults standardUserDefaults];
@@ -759,6 +761,16 @@ - (NSUInteger)metricExportInterval {
   return [configuredInterval unsignedIntegerValue];
 }
 
+// Returns a default value of 30 (for 30 seconds).
+- (NSUInteger)metricExportTimeout {
+  NSNumber *configuredInterval = self.configState[kMetricExportTimeout];
+
+  if (configuredInterval == nil) {
+    return 30;
+  }
+  return [configuredInterval unsignedIntegerValue];
+}
+
 - (NSDictionary *)extraMetricLabels {
   return self.configState[kMetricExtraLabels];
 }

Source/santametricservice/Writers/BUILD

@@ -39,6 +39,7 @@ objc_library(
     ],
     deps = [
         ":SNTMetricWriter",
+        "//Source/common:SNTConfigurator",
         "//Source/common:SNTLogging",
         "@MOLAuthenticatingURLSession",
     ],

Source/santametricservice/Writers/SNTMetricHTTPWriter.m

@@ -14,6 +14,7 @@
 #import <MOLAuthenticatingURLSession/MOLAuthenticatingURLSession.h>
 #include <dispatch/dispatch.h>
 
+#import "Source/common/SNTConfigurator.h"
 #import "Source/common/SNTLogging.h"
 #import "Source/santametricservice/Writers/SNTMetricHTTPWriter.h"
 
@@ -49,37 +50,45 @@ - (BOOL)write:(NSArray<NSData *> *)metrics toURL:(NSURL *)url error:(NSError **)
     dispatch_group_enter(requests);
 
     request.HTTPBody = (NSData *)value;
-    [[_session dataTaskWithRequest:request
-                 completionHandler:^(NSData *_Nullable data, NSURLResponse *_Nullable response,
-                                     NSError *_Nullable err) {
-                   if (err != nil) {
-                     _blockError = err;
-                     *stop = YES;
-                   } else if (response == nil) {
-                     *stop = YES;
-                   } else if ([response isKindOfClass:[NSHTTPURLResponse class]]) {
-                     NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)response;
-
-                     // Check HTTP error codes and create errors for any non-200.
-                     if (httpResponse && httpResponse.statusCode != 200) {
-                       _blockError = [[NSError alloc]
-                         initWithDomain:@"com.google.santa.metricservice.writers.http"
-                                   code:httpResponse.statusCode
-                               userInfo:@{
-                                 NSLocalizedDescriptionKey : [NSString
-                                   stringWithFormat:@"received http status code %ld from %@",
-                                                    httpResponse.statusCode, url]
-                               }];
-                       *stop = YES;
-                     }
-                   }
-                   dispatch_group_leave(requests);
-                 }] resume];
-
-    // Wait up to 30 seconds for the request to complete.
-    if (dispatch_group_wait(requests, (int64_t)(30.0 * NSEC_PER_SEC)) != 0) {
+    NSURLSessionDataTask *task = [_session
+      dataTaskWithRequest:request
+        completionHandler:^(NSData *_Nullable data, NSURLResponse *_Nullable response,
+                            NSError *_Nullable err) {
+          if (err != nil) {
+            _blockError = err;
+            *stop = YES;
+          } else if (response == nil) {
+            *stop = YES;
+          } else if ([response isKindOfClass:[NSHTTPURLResponse class]]) {
+            NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)response;
+
+            // Check HTTP error codes and create errors for any non-200.
+            if (httpResponse && httpResponse.statusCode != 200) {
+              _blockError = [[NSError alloc]
+                initWithDomain:@"com.google.santa.metricservice.writers.http"
+                          code:httpResponse.statusCode
+                      userInfo:@{
+                        NSLocalizedDescriptionKey :
+                          [NSString stringWithFormat:@"received http status code %ld from %@",
+                                                     httpResponse.statusCode, url]
+                      }];
+              *stop = YES;
+            }
+          }
+          dispatch_group_leave(requests);
+        }];
+
+    [task resume];
+
+    SNTConfigurator *config = [SNTConfigurator configurator];
+    int64_t timeout = (int64_t)config.metricExportTimeout;
+
+    // Wait up to timeout seconds for the request to complete.
+    if (dispatch_group_wait(requests, (timeout * NSEC_PER_SEC)) != 0) {
+      [task cancel];
       NSString *errMsg =
-        [NSString stringWithFormat:@"HTTP request to %@ timed out after 30 seconds", url];
+        [NSString stringWithFormat:@"HTTP request to %@ timed out after %lu seconds", url,
+                                   (unsigned long)timeout];
 
       _blockError = [[NSError alloc] initWithDomain:@"com.google.santa.metricservice.writers.http"
                                                code:ETIMEDOUT

Source/santametricservice/Writers/SNTMetricHTTPWriterTest.m

@@ -4,6 +4,7 @@
 #import <MOLAuthenticatingURLSession/MOLAuthenticatingURLSession.h>
 #import <OCMock/OCMock.h>
 
+#import "Source/common/SNTConfigurator.h"
 #import "Source/santametricservice/Writers/SNTMetricHTTPWriter.h"
 
 @interface SNTMetricHTTPWriterTest : XCTestCase
@@ -12,6 +13,7 @@ @interface SNTMetricHTTPWriterTest : XCTestCase
 @property id mockMOLAuthenticatingURLSession;
 @property NSMutableArray<NSDictionary *> *mockResponses;
 @property SNTMetricHTTPWriter *httpWriter;
+@property id mockConfigurator;
 @end
 
 @implementation SNTMetricHTTPWriterTest
@@ -28,6 +30,9 @@ - (void)setUp {
   self.httpWriter = [[SNTMetricHTTPWriter alloc] init];
   self.mockResponses = [[NSMutableArray alloc] init];
 
+  self.mockConfigurator = OCMClassMock([SNTConfigurator class]);
+  OCMStub([self.mockConfigurator configurator]).andReturn(self.mockConfigurator);
+
   // This must be marked __unsafe_unretained because we're going to store into
   // it using NSInvocation's getArgument:atIndex: method which takes a void*
   // to populate. If we don't mark the variable __unsafe_unretained it will
@@ -181,4 +186,18 @@ - (void)testEnsurePassingNilOrNullErrorDoesNotCrash {
 
   XCTAssertFalse(result);
 }
-@end
+
+- (void)testEnsureTimeoutsDoNotCrashWriter {
+  NSURL *url = [NSURL URLWithString:@"http://localhost:11444"];
+
+  // Queue up two responses for nil and NULL.
+  [self createMockResponseWithURL:url withCode:400 withData:nil withError:nil];
+  // Set the timeout to 0 second
+  OCMStub([self.mockConfigurator metricExportTimeout]).andReturn(0);
+
+  NSData *JSONdata = [@"{\"foo\": \"bar\"}\r\n" dataUsingEncoding:NSUTF8StringEncoding];
+
+  BOOL result = [self.httpWriter write:@[ JSONdata ] toURL:url error:nil];
+  XCTAssertEqual(NO, result);
+}
+@end

docs/deployment/configuration.md

@@ -56,7 +56,8 @@ also known as mobileconfig files, which are in an Apple-specific XML format.
 | EnableMachineIDDecoration     | Bool       | If YES, this appends the MachineID to the end of each log line. Defaults to NO.       |
 | MetricFormat                  | String     | Format to export metrics as, supported formats are "rawjson" for a single JSON blob and "monarchjson" for a format consumable by Google's Monarch tooling. Defaults to "". |
 | MetricURL                     | String     | URL describing where monitoring metrics should be exported.  |
-| MetricExportInterval          | Integer    | Number of seconds to wait between exporting metrics. Defaults to 30.  
+| MetricExportInterval          | Integer    | Number of seconds to wait between exporting metrics. Defaults to 30.  |
+| MetricExportTimeout           | Integer    | Number of seconds to wait before a timeout occurs when exporting metrics. Defaults to 30.  |
 | MetricExtraLabels             | Dictionary | A map of key value pairs to add to all metric root labels. (e.g. a=b,c=d) defaults to @{}). If a previously set key (e.g. host_name is set to "" then the key is remove from the metric root labels. Alternatively if a value is set for an existing key then the new value will override the old. |