Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use connection manager for keepalives #20

Merged

Conversation

DavidStirling
Copy link
Member

@DavidStirling DavidStirling commented Jul 31, 2024

Fixes #18

If you don't close sessions aggressively the omero-py keepalive system currerntly doesn't shut down properly and hangs Python on exit (we're trying to use an atexit handler to manage sessions). To fix this I've added our own keepalive ping system which makes use of the connection tracker we already have, which should do until the underlying issues in omero-py are resolved.

Basic test script:

import omero2pandas
import time

print("Starting test")
connector = omero2pandas.connect_to_omero(allow_token=False, server=<server>, username=<user>, password=<pwd>)
client = connector.get_client()
print("Connected:", connector.connected)
time.sleep(5)
print("Complete, shutting down!")

Should fail to exit without PR, but exit properly with.

Copy link
Member

@chris-allan chris-allan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this. It effectively recapitulates and effectively works against what the omero-py client object and service factory already do. We should talk this through more to understand what's going on.

@chris-allan
Copy link
Member

Relevant upstream PR is ome/omero-py#424.

Copy link
Member

@chris-allan chris-allan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to approve this for now so it can go in temporarily for as long as it's required.

We will need to work to get ome/omero-py#424 in as soon as possible and then follow up here refactoring as required and depending explicitly on the omero-py version that includes that change.

This PR will be for @erindiel, @sbesson and @emilroz to get integrated and shipping as required for other work next week.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reproduce the exit issue using the existing omero2pandas and latest omero-py code. With the proposed changes, I confirm the test script exits as expected rather than hanging.

There is a lot of logic injected in connect.py and I have not assessed all the conditions of error but it is my understanding this is meant as a temporary workaround so also proposing to get this into the next omero2pandas release and focus on getting the omero-py changes reviewed under all scenarios and merged so that we can revert to using the upstream keep alive logic.

@DavidStirling DavidStirling merged commit cb63103 into glencoesoftware:main Aug 26, 2024
6 checks passed
@sbesson
Copy link
Member

sbesson commented Sep 16, 2024

omero-py 5.19.5 has just been released with the corresponding upstream fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EnableKeepAlive when joining sessions
3 participants