Fix LegacyImageMap's ImagePlus references

[hinerm]

Currently, the LegacyImageMap has the following issues regarding how it handles ImagePlus references:

• Multiple WeakReferences. A Set of WeakReferences is used, but each WeakReference is unique so it's always added to the set. What we want is a WeakSet effectively. See this SO question for suggestions (such as using a WeakHashMap and Collections.newSetFromMap).
• Use of ConcurrentHashMap. This creates strong references to the ImagePluses which prevents them from being garbage collected.

One step in the desired direction could be to javassist the ImagePlus#close method to deregister an ImagePlus from the LegacyImageMap. However, the ImageJ macro language does not actually close an ImagePlus using the close() method.

So it seems like we either need to do more aggressive javassisting or move to WeakReferences.

If we do go to full WeakReferences we will also need to disable modern mode, as we will no longer be able to keep references to ImagePluses in modern mode. In legacy mode, the WindowManager is what keeps ImagePluses from being garbage collected. We could populate this structure in modern mode.. but it wouldn't work when headless. So we also need a better understanding of the legacy mappings when headless to properly determine how to proceed.

[ctrueden]

Right now, if you open an image, close it, then open it again, it will have -1 appended to the title the second time (and -2 the third time, etc.). This might be a symptom of this issue, or it might be a separate bug. For now, I mention it here so we can test as we are fixing this issue. If the problem remains after addressing the LegacyImageMap leakage, we can file a separate issue for it.

[dscho]

Right now, if you open an image, close it, then open it again, it will have -1 appended to the title the second time (and -2 the third time, etc.)

My analysis revealed that the behavior is due to DefaultImageDisplay asking the DisplayService when determining a unique name which in turn asks the ObjectService whether there exists a display of that name.

That is all correct, except that it looks as if the display object is never removed from the ObjectService when an ImagePlus is closed.

To help debugging this further, I made a unit test.

[ctrueden]

So @dscho, to be clear: you think the LegacyImageMap does always have its linkages broken down at the appropriate times? That is: this issue is not really about the LegacyImageMap but rather about the ImageDisplay objects not being disposed properly?

As discussed in person, I will tackle the disposal of Dataset, DatasetView, ImageDisplay et. al and generally clean up that code. Assuming I do all that, and the ObjectService no longer has any dangling references, is there also an issue with the LegacyImageMap retaining linkages as @hinerm suggests? Or do you think our problems will all happily disappear?

[dscho]

you think the LegacyImageMap does always have its linkages broken down at the appropriate times?

At the moment, it does not. Due to the legacy thread group hacks that do not work with the way we call commands at all.

I have some hacky removal in place but it needs a lot of cleaning up and a lot of testing before I am comfortable with merging it into master.

is there also an issue with the LegacyImageMap retaining linkages as @hinerm suggests?

I think with your work, my clean up should completely get rid of the references.

[dscho]

@hinerm sorry for leaving this so much in limbo and cryptic... The changes I made are in one monster wip commit on the image-suffix-wip branch...

[hinerm]

@dscho no apologies necessary. Just wanted to know where things were at so I can start working on it :)

[hinerm]

@dscho @ctrueden apologies that I was unable to understand exactly what was trying to be accomplished in the image-suffix-wip branch. I looked at LegacyImageMap again today and decided to resolve the conflation of concepts contained therein. see: https://github.com/imagej/imagej-legacy/tree/image-tracking

I'll look at image-suffix-wip more tomorrow and try to reconcile the two..

[dscho]

@hinerm oh, thank you! I did not get around to split things up, and the only reason this is on image-suffix-wip instead of a better named branch is that I ran out of time, and then out of the office ;-)

[hinerm]

This is fixed now as of 6d8faf3

Closing. @dscho could you open another issue for anything else image-suffix-wip is intended to address?