Session ID and constructor

[jburel]

Specifying a client as a parameter leads to strange behaviour when attempting to access session or connect Looking at the constructor of the BlitzGateway, the connected flag is set even if a client without session is specified.

This leads to error like

omero.RemovedSessionException: exception ::omero::RemovedSessionException
{
serverStackTrace = ome.conditions.RemovedSessionException: No context for
at ome.services.sessions.state.SessionCache.getDataNullOrThrowOnTimeout(SessionCache.java:432)
at ome.services.sessions.state.SessionCache.getSessionContext(SessionCache.java:368)

See https://github.com/TheJacksonLaboratory/ezomero/issues/100 for background

This PR:

1. Sets the host and port from the client if the client is passed a parameter
2. Sets the session and sessionID if a "truly" connected client is passed
3. Makes sure that the session is not closed if associated to passed client when connect is invoked. In that case a new client is created and the Gateway joins the session

To test: Check that the sessionID is set.

client = omero.client(server)
client.createSession(user, password)
sid = client.getSessionId()
with BlitzGateway(client_obj=client) as conn:
    assert sid == conn.getSession().getUuid().val

Check that the session is not closed

client=omero.client(server)
client.createSession(user, password)
with BlitzGateway(client_obj=client) as conn:
    assert conn.connect(), "Session is closed"

Check you can connect after setting the identity

client=omero.client(server)
with BlitzGateway(client_obj=client) as conn:
   conn.setIdentity(user, password)
    assert conn.connect(), "New session cannot be created"

client=omero.client(server)
c=omero.client(server)
c.createSession(user1, password1)
with BlitzGateway(client_obj=client) as conn:
   assert Uuid=c.getSessionId(), "Session cannot be joined"

client=omero.client(server)
c=omero.client(server1)
c.createSession(user1, password1)
with BlitzGateway(client_obj=client) as conn:
   asserts Uuid != c.getSessionId(), "Different host, should not work"

Note that none of the code snippets work if the host is not retrieved from the specified client. In order to work the host parameter needs to be set

[jburel]

I was thinking about similar issues last night and I agree that it requires some investigations

[jburel]

From the _init_ method

if self.c is None:
            self._resetOmeroClient()
        else:
            # if we already have client initialised, we can go ahead and create
            # our services.
            self._connected = True
            self._createProxies()
            self.SERVICE_OPTS = self.createServiceOptsDict()

This indicates that a connection is established so the client should have an established connection but nothing is set. This seems to be contradicting the description of the method

[DavidStirling]

I'd also been looking into this and came to a similar conclusion.

Fetching the UUID in the getSession() function seems to fix this particular case, but I wonder if it may be worth instead checking for and setting _sessionUuid within the init function when a pre-existing client is passed in. This would also provide an opportunity to validate whether the client is actually set up/connected properly.

If my understanding is correct this would then allow users to join or rejoin the session by calling conn.connect() if anything goes wrong, which currently fails and/or tries to create a new session.

[jburel]

Digging a bit more into the problem The reason that

with BlitzGateway(client_obj=client) as conn:
  conn.connect()

returns False is due to the fact that information from the passed client are not taken into account host when a new client is created (if session_id is None) and username and password when the session is created

I use the following code snippet to validate

with BlitzGateway(client_obj=client, host=server) as conn:
    conn.setIdentity(user, password)
    print(conn.connect())

This will return True

If self._sessionUuid is not none, then a new client does not need to be created and the session id is used to "rejoin" setting thesessionUuidwill solve one problem but it will not solve the assumption made in_resetOmeroClient`` about the host if a client with host is specified.

[jburel]

Setting the sessionUuid from the client if available leads to other problems when testing via

client=omero.client(server)
client.createSession(user, password)
with BlitzGateway(client_obj=client) as conn:
  conn.connect()

error

exception ::Glacier2::PermissionDeniedException
{
    reason = Internal error. Please contact your administrator:
 Wrapped Exception: (org.springframework.ldap.AuthenticationException):
[LDAP: error code 49 - INVALID_CREDENTIALS: Bind failed: ERR_229 Cannot authenticate user uid=admin,ou=system]; nested exception is javax.naming.AuthenticationException: [LDAP: error code 49 - INVALID_CREDENTIALS: Bind failed: ERR_229 Cannot authenticate user uid=admin,ou=system]
}

No idea about this ldap error since I am not using a LDAP user so not there yet

[jburel]

client=omero.client(server)
client.createSession(user, password)
print(client.getSessionId())
c=omero.client(server)
c.joinSession(client.getSessionId())
print(c.getSessionId())

This works fine so something in the gateway is happening

[jburel]

I found the problem: if the sessionUuid is set, in the connect method

• the client is deleted in _resetOmeroClient(self) so the session is no longer valid
• leading to the error when attempting to join that session

[jburel]

The regression is possibly due to the fact that the session is not discarded if it is coming from the client. In that case the contract will have to be different It seems that the "flexibility" of the constructor leads to the fact that some methods cannot be invoked depending on the constructor used.

[jburel]

I have updated the description with various scenarii I have tried

[will-moore]

There is a test failing at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-test-integration/lastCompletedBuild/testReport/OmeroPy.test.integration.gatewaytest.test_chmod/TestDefaultSetup/testAuthorCanEdit/ I can't be sure whether it is due to this PR - I haven't yet managed to get integration tests running on my local machine, so I can't test there. Since @jburel is away just now, he suggests I exclude to see if that fixes the test for now...

[jburel]

@DavidStirling I have added integration tests for the proposed changes (all green) Anything else you came across during your investigation?

[DavidStirling]

LGTM, thanks

[jburel]

Integration tests are green but I could add a few more to match your detailed review

[jburel]

Supporting tests are now all green. Proposing to merge and release as 5.19.2 (alongside a few approved PRs)

[sbesson]

Supporting tests are now all green. Proposing to merge and release as 5.19.2 (alongside a few approved PRs)

No objection from my side. Do you have a shortlist of all PRs proposed for inclusion?

[jburel]

short list:

394

398

406

405 (potentially but in that case tag will be 5.20.0)

[will-moore]

Would be great to get all those PRs in 👍

[sbesson]

Looks good to me. I am also in favor of including #405 as it reduces the possibility of calls which can cause memory issues server-side. As discussed on the PR, the public API is not modified so I think it could be amenable for either a patch or minor release increment.