Add option to create new ThumbnailStore connection

[dominikl]

Just adds an option to _prepareTB to use a new ThumbnailStore connection. If the cached one of the BlitzGateway is used then you can't call getThumbnail for multiple images in parallel.

Required by ome/omero-cli-render#33

[joshmoore]

In general 👍 for the added functionality. The only thing that it would be good to consider is if there is any unification needed in terms of the API if we foresee ourselves adding something similar to other methods. cc: @will-moore

A few ideas:

• drop "_ts"
• if there are other similar arguments somewhere, match them
• perhaps make this global state on the conn object? e.g. conn.cacheServices(False)

but probably could use some discussion before any more commits, @dominikl.

[will-moore]

You don't need this if the default use_cached_ts is True.

[dominikl]

Not quite sure. So only def getThumbnail(self, size=(64, 64), z=None, t=None, direct=True, rdefId=None, use_cached_ts) then check with if not use_cached_ts?

[joshmoore]

It wouldn't surprise me that the same argument would need to be present on multiple methods for a value to be propagated through a call stack (if it's not stateful).

[manics]

This is related: https://trello.com/c/lC8hFFix/522-blitzgatewaycreaterawfilestore-should-support-multiple-rawfilestores

[dominikl]

With the last commit you can set cacheServices to False on the Blitzgateway itself. Then calling methods like getThumbnailStore will use a new connection.

[joshmoore]

>>> BG1 = omero.gateway.BlitzGateway(...secret..., secure=True)
>>> BG1.connect()
True

# Initially gives the same value
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c048>
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c048>

# Can be modified to give different values
>>> BG1.cacheServices(False)
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c4a8>
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c518>

# Can be reverted back to the original behavior
# BUT: note that the returned value is the very first one!
>>> BG1.cacheServices(True)
>>> BG1.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c048>

# Can be taught from the outset to give different values
>>> BG2 = omero.gateway.BlitzGateway(...secret..., secure=True, cache_services=False)
>>> BG2.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c470>
>>> BG2.getPixelsService()
<omero.gateway.ProxyObjectWrapper object at 0x7f273c15c400>

Functionality seems to work as expected. 👍 The only issue that I note is that when using the mutator (cacheServices(False)) the existing instances are still held on to. There might be a potential for someone to leave a service hanging around unclosed. Since the new behavior is not the default, this probably isn't an immediate concern. If we foresee that we might change this behavior and if that change would be breaking, then we might want to either fix it from the outset or at least clarify it in the docs.

[dominikl]

Would it be useful to keep track of these ProxyObjectWrappers and add another method like clearUncachedServices() (or something like that) which can be used to close all connections which have been created while cacheServices was set to False?

[joshmoore]

I'm not sure. As long as the docs are clear on who's responsible in each case. For example, I'm pretty sure we've hunted down the various "leaks" caused by not calling close on the cached services. It'd be a shame to introduce something similar. Ultimately, who owns the pointers?

[dominikl]

--exclude
I'll exclude that for now. Yes, it's not a good idea the open up the possibility to create leaks. I think the previous way to only allow this for the thumbailstore exceptionally (commit
e0a5126 ) is better/safer although maybe a bit 'dirty'. Ok to revert back to this commit?

[will-moore]

I ran into the need to NOT reuse rawPixelStore recently when multiple threads try to load pixel data simultaneously from napari - see https://forum.image.sc/t/napari-lazy-loading-from-omero/32292
So I think there will be more cases where this is needed.

[joshmoore]

Happy to discuss. Technically a new method should mean this is a minor bump, so 5.6.0 is a good target. Would having cacheServices(False) clear existing services suffice to prevent a leak?