Merge pull request #908 from owenv/ovoorhees/wrapper-lifetimes

Tie lifetimes of Swift wrappers to c++ command and tool objects

Author: owenv

examples/c-api/buildsystem/main.c

@@ -90,6 +90,7 @@ static llb_buildsystem_command_t*
 fancy_tool_create_command(void *context, const llb_data_t* name) {
   llb_buildsystem_external_command_delegate_t delegate;
   delegate.context = NULL;
+  delegate.destroy_context = NULL;
   delegate.get_signature = NULL;
   delegate.configure = NULL;
   delegate.start = fancy_command_start;

products/libllbuild/BuildSystem-C-API.cpp

@@ -704,6 +704,12 @@ class CAPITool : public Tool {
     return std::unique_ptr<Command>(
         (Command*) cAPIDelegate.create_custom_command(cAPIDelegate.context, key_p));
   }
+  
+  ~CAPITool() {
+    if (cAPIDelegate.destroy_context) {
+      cAPIDelegate.destroy_context(cAPIDelegate.context);
+    }
+  }
 };
 
 class CAPIExternalCommand : public ExternalCommand {
@@ -1198,6 +1204,12 @@ class CAPIExternalCommand : public ExternalCommand {
     }
     return sig;
   }
+  
+  ~CAPIExternalCommand() {
+    if (cAPIDelegate.destroy_context) {
+      cAPIDelegate.destroy_context(cAPIDelegate.context);
+    }
+  }
 };
 
 }

products/libllbuild/include/llbuild/buildsystem.h

@@ -630,6 +630,10 @@ typedef struct llb_buildsystem_tool_delegate_t_ {
 
   llb_buildsystem_command_t* (*create_custom_command)(void* context,
                                                       const llb_build_key_t* key);
+  
+  /// Callback a client may use to tear down data structures associated with the context
+  /// pointer.
+  void (*destroy_context)(void* context);
 
   // FIXME: Support dynamic tool commands.
 } llb_buildsystem_tool_delegate_t;
@@ -771,6 +775,10 @@ typedef struct llb_buildsystem_external_command_delegate_t_ {
   bool (*is_result_valid)(void* context,
                           llb_buildsystem_command_t* command,
                           const llb_build_value* value);
+  
+  /// Callback a client may use to tear down data structures associated with the context
+  /// pointer.
+  void (*destroy_context)(void* context);
 
 } llb_buildsystem_external_command_delegate_t;
 

products/llbuildSwift/BuildSystemBindings.swift

@@ -168,11 +168,6 @@ private final class ToolWrapper {
         self.tool = tool
     }
 
-    /// The owning list of all created commands.
-    //
-    // FIXME: This is unfortunate, we should be able to destroy these naturally.
-    private var commandWrappers: [CommandWrapper] = []
-
     func createCommand(_ name: UnsafePointer<llb_data_t>) -> OpaquePointer? {
         let command = tool.createCommand(stringFromData(name.pointee)) as ExternalCommand?
         return command.map { command in buildCommand(name, command) } ?? nil
@@ -198,9 +193,9 @@ private final class ToolWrapper {
 
     private func buildCommand(_ name: UnsafePointer<llb_data_t>, _ command: ExternalCommand) -> OpaquePointer? {
         let wrapper = CommandWrapper(command: command)
-        self.commandWrappers.append(wrapper)
         var _delegate = llb_buildsystem_external_command_delegate_t()
-        _delegate.context = Unmanaged.passUnretained(wrapper).toOpaque()
+        _delegate.context = Unmanaged.passRetained(wrapper).toOpaque()
+        _delegate.destroy_context = { Unmanaged<CommandWrapper>.fromOpaque(UnsafeRawPointer($0!)).release() }
         _delegate.configure = { BuildSystem.toCommandWrapper($0!).configure($1!, $2!, $3!, $4!) }
         _delegate.get_signature = { return BuildSystem.toCommandWrapper($0!).getSignature($1!, $2!) }
         _delegate.start = { return BuildSystem.toCommandWrapper($0!).start($1!, $2!, $3) }
@@ -1253,10 +1248,6 @@ public final class BuildSystem {
         return fsGetFileInfo(path, info)
     }
 
-    /// The owning list of all created tools.
-    //
-    // FIXME: This is unfortunate, we should be able to destroy these naturally.
-    private var toolWrappers: [ToolWrapper] = []
     private func lookupTool(_ name: UnsafePointer<llb_data_t>) -> OpaquePointer? {
         // Look up the named tool.
         guard let tool = delegate.lookupTool(stringFromData(name.pointee)) else {
@@ -1265,9 +1256,9 @@ public final class BuildSystem {
 
         // If we got a tool, save it and create an appropriate low-level instance.
         let wrapper = ToolWrapper(tool: tool)
-        self.toolWrappers.append(wrapper)
         var _delegate = llb_buildsystem_tool_delegate_t()
-        _delegate.context = Unmanaged.passUnretained(wrapper).toOpaque()
+        _delegate.context = Unmanaged.passRetained(wrapper).toOpaque()
+        _delegate.destroy_context = { Unmanaged<CommandWrapper>.fromOpaque(UnsafeRawPointer($0!)).release() }
         _delegate.create_command = { return BuildSystem.toToolWrapper($0!).createCommand($1!) }
         _delegate.create_custom_command = { return BuildSystem.toToolWrapper($0!).createCustomCommand($1!) }
 

unittests/CAPI/BuildSystem-C-API.cpp

@@ -141,6 +141,7 @@ static llb_buildsystem_command_t*
 depinfo_tester_tool_create_command(void *context, const llb_data_t* name) {
   llb_buildsystem_external_command_delegate_t delegate;
   delegate.context = NULL;
+  delegate.destroy_context = NULL;
   delegate.get_signature = NULL;
   delegate.configure = NULL;
   delegate.start = depinfo_tester_command_start;