Ver Fonte

Fix two huge memory leaks in `cgrpc_call` and `cgrpc_handler`. These would all GRPC call objects to never get released, which in turn caused their completion queues and associated file descriptors to never get released. This became apparent after ~3-5k requests on macOS.

Daniel Alm há 7 anos atrás
pai
commit
78a1291789

+ 3 - 1
Sources/CgRPC/shim/call.c

@@ -21,7 +21,9 @@
 #include <assert.h>
 
 void cgrpc_call_destroy(cgrpc_call *call) {
-  //grpc_call_destroy(call->call);
+  if (call->call) {
+    grpc_call_unref(call->call);
+  }
   free(call);
 }
 

+ 1 - 0
Sources/CgRPC/shim/channel.c

@@ -85,6 +85,7 @@ cgrpc_call *cgrpc_channel_create_call(cgrpc_channel *channel,
   // create call
   host_slice = grpc_slice_from_copied_string(host);
   gpr_timespec deadline = cgrpc_deadline_in_seconds_from_now(timeout);
+  // The resulting call will have a retain call of +1. We'll release it in `cgrpc_call_destroy()`.
   grpc_call *channel_call = grpc_channel_create_call(channel->channel,
                                                      NULL,
                                                      GRPC_PROPAGATE_DEFAULTS,

+ 10 - 3
Sources/CgRPC/shim/handler.c

@@ -38,7 +38,7 @@ void cgrpc_handler_destroy(cgrpc_handler *h) {
   grpc_metadata_array_destroy(&(h->request_metadata_recv));
   grpc_call_details_destroy(&(h->call_details));
   if (h->server_call) {
-    //grpc_call_destroy(h->server_call);
+    grpc_call_unref(h->server_call);
   }
   free(h);
 }
@@ -67,6 +67,10 @@ cgrpc_call *cgrpc_handler_get_call(cgrpc_handler *h) {
   cgrpc_call *call = (cgrpc_call *) malloc(sizeof(cgrpc_call));
   memset(call, 0, sizeof(cgrpc_call));
   call->call = h->server_call;
+  if (call->call) {
+    // This retain will be balanced by `cgrpc_call_destroy()`.
+    grpc_call_ref(call->call);
+  }
   return call;
 }
 
@@ -77,6 +81,11 @@ cgrpc_completion_queue *cgrpc_handler_get_completion_queue(cgrpc_handler *h) {
 grpc_call_error cgrpc_handler_request_call(cgrpc_handler *h,
                                            cgrpc_metadata_array *metadata,
                                            long tag) {
+  if (h->server_call != NULL) {
+    return GRPC_CALL_OK;
+  }
+  // This fills `h->server_call` with a call with retain count of +1.
+  // We'll release that retain in `cgrpc_handler_destroy()`.
   return grpc_server_request_call(h->server->server,
                                   &(h->server_call),
                                   &(h->call_details),
@@ -85,5 +94,3 @@ grpc_call_error cgrpc_handler_request_call(cgrpc_handler *h,
                                   h->server->completion_queue,
                                   cgrpc_create_tag(tag));
 }
-
-

+ 1 - 1
Sources/SwiftGRPC/Core/Handler.swift

@@ -32,7 +32,7 @@ public class Handler {
   /// A Call object that can be used to respond to the request
   public private(set) lazy var call: Call = {
     Call(underlyingCall: cgrpc_handler_get_call(self.underlyingHandler),
-         owned: false,
+         owned: true,
          completionQueue: self.completionQueue)
   }()
 

+ 3 - 3
Sources/SwiftGRPC/Core/Metadata.swift

@@ -69,13 +69,13 @@ public class Metadata: CustomStringConvertible {
   }
   
   public var description: String {
-    var result = ""
+    var lines: [String] = []
     for i in 0..<count() {
       let key = self.key(i)
       let value = self.value(i)
-      result += (key ?? "(nil)") + ":" + (value ?? "(nil)") + "\n"
+      lines.append((key ?? "(nil)") + ":" + (value ?? "(nil)"))
     }
-    return result
+    return lines.joined(separator: "\n")
   }
   
   public func copy() -> Metadata {

+ 10 - 6
Sources/SwiftGRPC/Runtime/ServiceServer.swift

@@ -22,6 +22,8 @@ open class ServiceServer {
   public let address: String
   public let server: Server
 
+  public var shouldLogRequests = true
+
   /// Create a server that accepts insecure connections.
   public init(address: String) {
     gRPC.initialize()
@@ -58,13 +60,15 @@ open class ServiceServer {
         return
       }
 
-      let unwrappedHost = handler.host ?? "(nil)"
       let unwrappedMethod = handler.method ?? "(nil)"
-      let unwrappedCaller = handler.caller ?? "(nil)"
-      print("Server received request to " + unwrappedHost
-        + " calling " + unwrappedMethod
-        + " from " + unwrappedCaller
-        + " with " + handler.requestMetadata.description)
+      if strongSelf.shouldLogRequests == true {
+        let unwrappedHost = handler.host ?? "(nil)"
+        let unwrappedCaller = handler.caller ?? "(nil)"
+        print("Server received request to " + unwrappedHost
+          + " calling " + unwrappedMethod
+          + " from " + unwrappedCaller
+          + " with " + handler.requestMetadata.description)
+      }
       
       do {
         if !(try strongSelf.handleMethod(unwrappedMethod, handler: handler, queue: queue)) {

+ 15 - 0
Tests/SwiftGRPCTests/EchoTests.swift

@@ -22,6 +22,7 @@ class EchoTests: BasicEchoTestCase {
   static var allTests: [(String, (EchoTests) -> () throws -> Void)] {
     return [
       ("testUnary", testUnary),
+      ("testUnaryLotsOfRequests", testUnaryLotsOfRequests),
       ("testClientStreaming", testClientStreaming),
       ("testClientStreamingLotsOfMessages", testClientStreamingLotsOfMessages),
       ("testServerStreaming", testServerStreaming),
@@ -48,6 +49,20 @@ extension EchoTests {
     XCTAssertEqual("Swift echo get: foo", try! client.get(Echo_EchoRequest(text: "foo")).text)
     XCTAssertEqual("Swift echo get: foo", try! client.get(Echo_EchoRequest(text: "foo")).text)
   }
+  
+  func testUnaryLotsOfRequests() {
+    // No need to spam the log with 50k lines.
+    server.shouldLogRequests = false
+    let clockStart = clock()
+    let numberOfRequests = 50_000
+    for i in 0..<numberOfRequests {
+      if i % 1_000 == 0 {
+        print("\(i) requests sent so far, elapsed time: \(Double(clock() - clockStart) / Double(CLOCKS_PER_SEC))")
+      }
+      XCTAssertEqual("Swift echo get: foo \(i)", try client.get(Echo_EchoRequest(text: "foo \(i)")).text)
+    }
+    print("total time for \(numberOfRequests) requests: \(Double(clock() - clockStart) / Double(CLOCKS_PER_SEC))")
+  }
 }
 
 extension EchoTests {