Close all HandleTie servants created during import #354
Conversation
This copies the logic from the command-line importer library to ensure the ImportLibrary Callback can close the handle
|
This indicates that https://github.com/ome/omero-insight/blob/master/src/main/java/org/openmicroscopy/shoola/agents/fsimporter/util/FileImportComponent.java#L408 and https://github.com/ome/omero-insight/blob/master/src/main/java/org/openmicroscopy/shoola/agents/fsimporter/util/LightFileImportComponent.java#L136 are not reached for the last import and could potentially be removed |
|
Could |
|
Yes, looks like the same lines of code in the (Light)FileImportComponent could be removed then. |
|
Based on the comments above, would you consider the issue should be fixed in the In the case of Insight, the API is returning the callback which is used downstream by |
|
I'm fine with the change. Better to do it in the OMEROGateway class than the ImportComponents, I think. I was just wondering, if |
No, since the |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/memory-note-clearing-after-data-imports/78815/2 |
| @@ -5861,6 +5863,9 @@ Object importImageFile(SecurityContext ctx, ImportableObject object, | |||
| } catch (Exception ex) {} | |||
| if (omsc != null && close) | |||
| closeImport(ctx, userName); | |||
| if (cb != null) { | |||
There was a problem hiding this comment.
Realizing (too late!) it's surprising this even works!
There was a problem hiding this comment.
Thanks for the sanity check. That echoes my concerns expressed in #354 (comment).
Retesting my commit with a local server, I confirm the handle closure in the finally block causes the UI to hang with a spinning wheel. That happens both for fake single image and fake plates with 4 wells. Interestingly, I am also unable to reproduce the original issue of leaking HandleTie servants with OMERO.insight 5.7.2.
Interesting I have a different behavior when connecting to a remote server (same OMERO.server version) and against which I conducted my initial testing for this PR:
- with OMERO.insight 5.7.2, I can reproduce the unclosed servant issue with both the single fake images and the fake 4-well plate
- with this change included, the last handle is unregistered in both cases and the import completes successfully
- with this change included, if importing a 16 wells plate, the servant get unregistered but the import is left spinning forever
I'll mark this PR as draft as more investigation is needed.
There was a problem hiding this comment.
Imo it doesn't really matter where cb.close() is called, either directly after creating the callback or in the finally block (later just make is unncessary complicated I think). Anyway, I wonder how it affects Insight's counting of the finished/active imports (pretty sure that's why the spinner doesn't stop).
There was a problem hiding this comment.
Imo the cb instance should only be called by the creating thread if there's an exception since when the exception is thrown the cb instance won't be returned and therefore no one will clean up the server-side reference.
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/continously-high-ram-usage-of-omero-server-after-large-import/79555/2 |
|
Came back to this within the context of the testing of https://forum.image.sc/t/first-release-of-omero-process-container-steward/85067. I confirm that the issue persists using OMERO.insight 5.8.1 against a server including the When importing multiple files, the last created |
|
imported 2 images against a remote server |
|
Retested against OME demo server, imported a The last 5 servant unregistrations happened after terminating the OMERO.insight connection. So in the example above that's ~50% of the imports where some servants are not cleaned before the session termination |
|
Did you import with some options off e.g. no thumbnails? |
No options. Regular import of a dummy test.fake file |
|
this is what I have with the last commit |
|
Same outcome for me testing either against the OME demo service hosted at UoD and against a GS testing instance hosted on Adding some debugging statements, I am getting increasingly convinced the primary problem is that is not systematically called for every import. In case this line is never accessed, the callback is not registered and there is no logic to close it incl. all associated servants until the termination of the session. |
|
I think I know what's happening |
|
I'm tempted to say, if it works 👍 Insight code just got so complicated (I'm well aware that I've contributed to that...). It definitely makes sense to move that out of the Swing thread to make sure the callback is set. |
|
Retested from bafe42a against a remote server with indivual import of fake files. For each file, the HandleTie servant was left uncleaned and only unregistered at session termination |
|
thanks |
|
replaced by #394 |
This addresses an issue with associated with dangling servants left at the end of a data import. This PR copies over the minimal callback closing changes from the
ImportLibrary.importImageand more specifically https://github.com/ome/omero-blitz/blob/49c5d46042487014d0dd6753679cf7f15bee873b/src/main/java/ome/formats/importer/ImportLibrary.java#L704-L709 to ensure that all handle servants are unregistered server-side.To test these changes, run an import of one or multiple filesets and watch the creation/deletion of
HandleTieservants server-side e.g. usingtail -F Blitz-0.log | grep HandleTie.omero.cmd._HandleTieservants, one per fileset, and only close N-1. The last servant should be created (Added servant...) but never deleted (Unregistered servant...)