Merge pull request #269 from hamishknight/timedout

Merge `timedOut` with `softTimeout`

Author: hamishknight

SourceKitStressTester/Sources/Common/Message.swift

@@ -39,11 +39,6 @@ public struct SourceKitResponseData: Codable {
 public enum SourceKitError: Error {
   case crashed(request: RequestInfo)
   case timedOut(request: RequestInfo)
-  /// Thrown if a request was close to the time out that triggers a timedOut
-  /// error.
-  /// If it was counted how many instructions SourceKit took to fulfill the
-  /// request, `instructions` contains that number. Otherwise it's `nil`.
-  case softTimeout(request: RequestInfo, duration: TimeInterval, instructions: Int?)
   case failed(_ reason: SourceKitErrorReason, request: RequestInfo, response: String)
 
   public var request: RequestInfo {
@@ -52,25 +47,26 @@ public enum SourceKitError: Error {
       return request
     case .timedOut(let request):
       return request
-    case .softTimeout(let request, _, _):
-      return request
     case .failed(_, let request, _):
       return request
     }
   }
 
   /// Soft errors are allowed to match XFails, but don't fail the stress tester
   /// on their own.
-  /// The current use case for soft errors are soft timeouts, where the request
-  /// took more than half of the allowed time. If the issue is XFailed, we don't
-  /// want to mark it as unexpectedly passed because the faster execution time
-  /// might be due to noise. But we haven't surpassed the limit either, so it
-  /// shouldn't be a hard error either.
+  ///
+  /// The current use case for soft errors are timeouts, since it's possible
+  /// that it hits a failure on a faster run, which can lead to non-determinism
+  /// in matching XFAILs.
+  ///
+  /// Timouts aren't considered errors on their own since they would be a major
+  /// cause of stress tester failures, producing noise. We instead use
+  /// instruction count measurements to keep track of performance.
   public var isSoft: Bool {
     switch self {
-    case .crashed, .timedOut, .failed:
+    case .crashed, .failed:
       return false
-    case .softTimeout:
+    case .timedOut:
       return true
     }
   }
@@ -210,7 +206,7 @@ extension SourceKitError: Codable {
     case error, kind, request, response, duration, instructions
   }
   enum BaseError: String, Codable {
-    case crashed, failed, timedOut, softTimeout
+    case crashed, failed, timedOut
   }
 
   public init(from decoder: Decoder) throws {
@@ -222,11 +218,6 @@ extension SourceKitError: Codable {
     case .timedOut:
       let request = try container.decode(RequestInfo.self, forKey: .request)
       self = .timedOut(request: request)
-    case .softTimeout:
-      let request = try container.decode(RequestInfo.self, forKey: .request)
-      let duration = try container.decode(Double.self, forKey: .duration)
-      let instructions = try container.decodeIfPresent(Int.self, forKey: .instructions)
-      self = .softTimeout(request: request, duration: duration, instructions: instructions)
     case .failed:
       let reason = try container.decode(SourceKitErrorReason.self, forKey: .kind)
       let request = try container.decode(RequestInfo.self, forKey: .request)
@@ -244,11 +235,6 @@ extension SourceKitError: Codable {
     case .timedOut(let request):
       try container.encode(BaseError.timedOut, forKey: .error)
       try container.encode(request, forKey: .request)
-    case .softTimeout(let request, let duration, let instructions):
-      try container.encode(BaseError.softTimeout, forKey: .error)
-      try container.encode(request, forKey: .request)
-      try container.encode(duration, forKey: .duration)
-      try container.encodeIfPresent(instructions, forKey: .instructions)
     case .failed(let kind, let request, let response):
       try container.encode(BaseError.failed, forKey: .error)
       try container.encode(kind, forKey: .kind)
@@ -537,14 +523,6 @@ extension SourceKitError: CustomStringConvertible {
         \(markSourceLocation(of: request) ?? "<unmodified>")
         -- end file content ----------
         """
-    case .softTimeout(let request, let duration, let instructions):
-      return """
-        Request took \(duration) seconds (\(instructions.map(String.init) ?? "<unknown>") instructions) to execute. This is more than a tenth of the allowed time. This error will match XFails but won't count as an error by itself.
-          request: \(request)
-        -- begin file content --------
-        \(markSourceLocation(of: request) ?? "<unmodified>")
-        -- end file content ----------
-        """
     case .failed(let reason, let request, let response):
       return """
         \(reason)

SourceKitStressTester/Sources/StressTester/SourceKitDocument.swift

@@ -492,26 +492,17 @@ class SourceKitDocument {
       }
     }
 
-    do {
-      let response = try sendWithTimeout(request, info: info, validateResponse: validateResponse)
-      return (response, try getInstructionCount())
-    } catch SourceKitError.softTimeout(request: let request, duration: let duration, instructions: _) {
-      throw SourceKitError.softTimeout(request: request, duration: duration, instructions: try getInstructionCount())
-    } catch {
-      throw error
-    }
+    let response = try sendWithTimeout(request, info: info, validateResponse: validateResponse)
+    return (response, try getInstructionCount())
   }
 
   /// Send the `request` synchronously, timing out after
   /// `SOURCEKIT_REQUEST_TIMEOUT`.
   /// An error is thrown if either the response is invalid (see `throwIfInvalid`)
-  /// or `validateResponse` throws. If the request took longer than half the
-  /// time allowed but no other error is thrown, a `SourceKitError.softTimeout`
-  /// will be thrown.
+  /// or `validateResponse` throws.
   private func sendWithTimeout(_ request: SourceKitdRequest, info: RequestInfo, validateResponse: (SourceKitdResponse) throws -> Void = { _ in }, reopenDocumentIfNeeded: Bool = true) throws -> SourceKitdResponse {
     var response: SourceKitdResponse? = nil
     let completed = DispatchSemaphore(value: 0)
-    let startDate = Date()
     connection.send(request: request) {
       response = $0
       completed.signal()
@@ -523,23 +514,14 @@ class SourceKitDocument {
         fatalError("nil response without timout being hit?")
       }
 
-      let requestDuration = Date().timeIntervalSince(startDate)
-
       if response.isError, response.description.contains("No associated Editor Document"), reopenDocumentIfNeeded {
         _ = try self.openOrUpdate(source: sourceState?.source)
         return try sendWithTimeout(request, info: info, validateResponse: validateResponse, reopenDocumentIfNeeded: false)
       } else {
         // Check if there was an error in the response
         try throwIfInvalid(response, request: info)
         try validateResponse(response)
-
-        if requestDuration > TimeInterval(SOURCEKIT_REQUEST_TIMEOUT) / 10 {
-          // There was no error in the response, but the request took too long
-          // throw a soft timeout.
-          throw SourceKitError.softTimeout(request: info, duration: requestDuration, instructions: nil)
-        } else {
-          return response
-        }
+        return response
       }
     case .timedOut:
       /// We timed out waiting for the response. Any following requests would

SourceKitStressTester/Sources/StressTester/StressTester.swift

@@ -126,16 +126,7 @@ public class StressTester {
           try report(document.moduleInterfaceGen())
         }
       } catch {
-        if case SourceKitError.softTimeout(request: let request, duration: _, instructions: let .some(instructions)) = error {
-          reportPerformanceMeasurement(request: request, instructions: instructions, reusingASTContext: nil)
-        }
-        if case SourceKitError.timedOut = error {
-          // Ignore timeout errors. In practice, we have always just added the timeouts to the XFails and keeping track
-          // of these timeouts is the major cause of stress tester failures, producing noise.
-          // We use instruction count measurements to keep track of performance.
-        } else {
-          errors.append(error)
-        }
+        errors.append(error)
       }
     }