Julia: Image/PeakList memory semantics (again)

Previous commit a6462e1f0 was still not right.  If the image gets freed,
the PeakList can be left with a dangling reference.  If the peak list
gets replaced at the C level (e.g. by running a new peak search), Julia
will have no way of knowing about it.

Instead of having the PeakList know if it's associated with an image,
it's better to have the Image know (at the C level) if it's responsible
for freeing the ImageFeatureList.  As soon as the ImageFeatureList is
exposed to the Julia GC via a PeakList object, it becomes Julia's
responsibility to free it.  The Julia Image structure contains a
reference to the Julia PeakList, to prevent this from happening until
either the image is freed or a new PeakList is substituted.  The two
references (Julia image.peaklist and C image->features) have to be kept
in sync, and we check image->features every time the peaklist is
requested.

4 files changed, 32 insertions, 24 deletions

diff --git a/julia/CrystFEL/src/image.jl b/julia/CrystFEL/src/image.jl
index 24c1c923..de79a556 100644
--- a/julia/CrystFEL/src/image.jl
+++ b/julia/CrystFEL/src/image.jl
@@ -36,6 +36,7 @@ mutable struct InternalImage
     peak_resolution::Cdouble
     peaklist::Ptr{InternalPeakList}
     ida::Ptr{Cvoid}
+    owns_peaklist::Cint
 end
 
 
@@ -63,11 +64,27 @@ function makecrystallist(listptr, n)
 end
 
 
+function getpeaklist(image)
+    idata = unsafe_load(image.internalptr)
+    if (image.peaklist === nothing) || (idata.peaklist != image.peaklist.internalptr)
+        if idata.peaklist != C_NULL
+            image.peaklist = PeakList(idata.peaklist)
+            # From now on, Julia is completely responsible for freeing the peaklist
+            idata.owns_peaklist = 0
+            unsafe_store!(image.internalptr, idata)
+        else
+            image.peaklist = nothing
+        end
+    end
+    return image.peaklist
+end
+
+
 function Base.getproperty(image::Image, name::Symbol)
     if name === :internalptr
         getfield(image, :internalptr)
     elseif name === :peaklist
-        getfield(image, :peaklist)
+        getpeaklist(image)
     else
         idata = unsafe_load(image.internalptr)
 
@@ -96,23 +113,13 @@ function set_peaklist(image, new_pl)
 
     assert_type(new_pl, PeakList)
 
-    if new_pl.in_image
-        throw(ArgumentError("PeakList is already in an image.  "*
-                            "Add a copy (use `deepcopy`) instead."))
-    end
-    new_pl.in_image = true
-    old_pl = swapfield!(image, :peaklist, new_pl)
-    if old_pl !== nothing
-        old_pl.in_image = false
-        # The old PeakList will now get GCed
-    end
-
     idata = unsafe_load(image.internalptr)
-    old_i = swapfield!(idata, :peaklist, new_pl.internalptr)
-    if old_pl === nothing || old_i == old_pl.internalptr
-        unsafe_store!(image.internalptr, idata)
-        # else someone else already set a new peaklist
+    if (idata.owns_peaklist == 0) && (idata.peaklist != C_NULL)
+        @ccall libcrystfel.image_feature_list_free(idata.peaklist::Ptr{InternalPeakList})::Cvoid
     end
+    idata.peaklist = new_pl.internalptr
+    idata.owns_peaklist = 0
+    unsafe_store!(image.internalptr, idata)
 
 end
 
diff --git a/julia/CrystFEL/src/peaklist.jl b/julia/CrystFEL/src/peaklist.jl
index a676a7bf..d4354977 100644
--- a/julia/CrystFEL/src/peaklist.jl
+++ b/julia/CrystFEL/src/peaklist.jl
@@ -16,7 +16,6 @@ mutable struct InternalPeakList end
 
 mutable struct PeakList
     internalptr::Ptr{InternalPeakList}
-    in_image   # if true, this PeakList belongs to an Image struct
 end
 
 
@@ -25,11 +24,8 @@ function PeakList()
     if out == C_NULL
         throw(ArgumentError("Failed to create peak list"))
     end
-    finalizer(PeakList(out, false)) do pl
-        if !pl.in_image
-            @ccall libcrystfel.image_feature_list_free(pl.internalptr::Ptr{InternalPeakList})::Cvoid
-            # else it belongs to the image structure
-        end
+    finalizer(PeakList(out)) do pl
+        @ccall libcrystfel.image_feature_list_free(pl.internalptr::Ptr{InternalPeakList})::Cvoid
     end
 end
 
@@ -39,7 +35,7 @@ function Base.deepcopy(peaklist::PeakList)
     if out == C_NULL
         throw(ErrorException("Failed to copy peak list"))
     end
-    PeakList(out, false)
+    PeakList(out)
 end
 
 function Base.length(peaklist::PeakList)
diff --git a/libcrystfel/src/image.c b/libcrystfel/src/image.c
index 3b4e04df..ab4518a9 100644
--- a/libcrystfel/src/image.c
+++ b/libcrystfel/src/image.c
@@ -1394,7 +1394,7 @@ void image_free(struct image *image)
 	int i, np;
 
 	if ( image == NULL ) return;
-	image_feature_list_free(image->features);
+	if ( image->owns_peaklist ) image_feature_list_free(image->features);
 	free_all_crystals(image);
 	spectrum_free(image->spectrum);
 	cffree(image->filename);
@@ -1464,6 +1464,7 @@ struct image *image_new()
 	image->bw = -1.0;
 	image->peak_resolution = -1.0;
 	image->features = NULL;
+	image->owns_peaklist = 1;
 
 	return image;
 }
diff --git a/libcrystfel/src/image.h b/libcrystfel/src/image.h
index beb66810..74ff6eee 100644
--- a/libcrystfel/src/image.h
+++ b/libcrystfel/src/image.h
@@ -190,6 +190,10 @@ struct image
 	/** Re-usable data array structure, or NULL if not used */
 	ImageDataArrays         *ida;
 
+	/** If set, then 'features' should be freed with the image.
+	 * Otherwise, it is managed externally (e.g. by Julia) */
+	int                      owns_peaklist;
+
 };
 
 #ifdef __cplusplus