Java gateway bugfixes #5513

dominikl · 2017-09-18T16:10:43Z

What this PR does

Guest login was used to determine if a username is actually a session id. With this PR a joinSession attempt is made, if the username matches UUID format.
The Facility.handleException() method silently 'swallowed' exceptions caused by connection issues. With this PR a DSOutofServiceException is thrown in these cases.
Adds JoinSessionCredentials to provide a specific class for session id logins.
Automatically remove stale connectors from the groupConnectorMap. That way the Gateway can continue working after a server restart without explicit dis- and reconnect.
Makes the Gateway AutoCloseable so that it can be used in try-with-resources blocks.
In various Facility methods, check arguments and return immediately when arguments not reasonable (Ids not provided, etc.).

Testing this PR

Check Java gateway integration tests.
Test that login via session id still works
Test that reconnect after connection loss still works. E. g. open Insight. Disconnect network. Try to browse a dataset. A connection warning should appear after a while. Reconnect the network. Insight should reconnect again, too, without crashing.
Continuously request some data from the server using the same Gateway connection (catch all exceptions). Stop and restart the server in between. While the server is not running you will get some exceptions. But when the server is ready again the Gateway should continue working as usual. See below for an example test client (*).

Related reading

https://trello.com/c/XKg3FisH/42-java-gateway
https://trello.com/c/JF3VjeWk/694-refactor-logincredentials
Context for Facility.handleException() and server restart issue: https://www.openmicroscopy.org/community/viewtopic.php?f=6&t=8367

--

*) Example client to use for testing server restart

LoginCredentials l = new LoginCredentials( ... );
Gateway g = new Gateway(new SimpleLogger());

ExperimenterData exp = g.connect(l);
SecurityContext ctx = new SecurityContext(exp.getGroupId());

while (true) {
    try {
        IObject obj = g.getFacility(BrowseFacility.class).findIObject(
                ctx, Image.class.getSimpleName(), 1);
        System.out.println(obj.getClass().getSimpleName() + " "
                + obj.getId().getValue());
    } catch (Throwable e) {
        e.printStackTrace();
    } finally {
        try {
            Thread.sleep(5000);
        } catch (InterruptedException e) {
        }
    }
}

joshmoore · 2017-09-21T11:06:46Z

components/blitz/src/omero/gateway/Gateway.java

@@ -142,7 +142,7 @@
    public static final String PROP_STATELESS_SERVICE_CREATED = "PROP_STATELESS_SERVICE_CREATED";

    /** Regex matching ICE Session ID */
-    private static final String ICE_SESSION_ID_REGEX = "[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}";
+    public static final String ICE_SESSION_ID_REGEX = "[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}";


Interesting. Might be worth moving this to an Ice constant and using this server-side as well. To data, I've always treated the session IDs as opaque, i.e. no assumptions could be made.

The documentation says that each Ice object has an UUID and so does the session ID look like, although I didn't find anything in the documentation which explicitely says that the session ID is an UUID, too.

Hm, actually I could also just use Java's builtin UUID.fromString() method.

although I didn't find anything in the documentation which explicitely says that the session ID is an UUID, too.

Exactly. It's not yet part of the contract.

joshmoore · 2017-09-21T11:08:57Z

components/tools/OmeroJava/test/integration/gateway/GatewayUsageTest.java

+        ExperimenterData root2 = gw2.connect(c2);
+        Assert.assertNotNull(root2);
+        Assert.assertEquals(gw2.getSessionId(root2), sessionId);
+        gw2.disconnect();


If something throws re: gw2, gw1 will not be closed which could hang the test.

Could make the Gateway AutoCloseable ( c31e719 ), would make things easier. Any objections?

No objections. 😃

dominikl · 2017-09-26T15:01:02Z

Just for reference, already noticed the problem with the 'swallowed' exceptions in 5.2 #4344 (comment) and Josh's idea of an 'ExceptionHandler', but forgot about it again. General exception handling overhaul would be a breaking change, but probably necessary, maybe for 5.5 then...

mtbc · 2017-10-12T10:20:08Z

components/blitz/src/omero/gateway/facility/BrowseFacility.java

@@ -130,7 +130,7 @@
    public Collection<DataObject> getHierarchy(SecurityContext ctx, Class rootType,
            List<Long> rootIDs, Parameters options)
            throws DSOutOfServiceException, DSAccessException {
-        if (rootType == null || CollectionUtils.isEmpty(rootIDs))
+        if (rootType == null)


The rootIDs.isEmpty() case will go okay?

Yes, same behaviour as roodIDs == null (get all).

Aha, maybe also mention in Javadoc, just add "or empty" or something.

dominikl · 2017-10-16T07:40:37Z

Not quite sure, ready to merge or relist for standup?

mtbc · 2017-10-16T07:41:34Z

Has it yet had any functional testing?

dominikl · 2017-10-16T07:45:32Z

Nope, doesn't look like it. I'll relist and maybe @pwalczysko can have a look when he can spare some time.

pwalczysko · 2017-10-17T08:34:06Z

All the four bullet points from the "Testing this PR" header passed.
Ready to merge FMPOV

joshmoore · 2017-11-23T15:10:49Z

Thanks, @dominikl. At some point, it would be interesting to review your changes here against the Python Gateway (i.e. properly specifying the string format for the session ids, etc.) Merging into the clisplit branch for further testing.

With ome#5513 the java gateway removes stale connectors (e.g. after server restart). So Insight can't use the gateway any longer to provide the information which images are currently viewed, the stale references are removed after server restart / reconnect.

jburel added develop ON_HOLD labels Sep 18, 2017

joshmoore reviewed Sep 21, 2017

View reviewed changes

dominikl force-pushed the java_gateway_fixes branch from 9a777c6 to c31e719 Compare September 21, 2017 15:01

dominikl force-pushed the java_gateway_fixes branch from 1002462 to f20bf97 Compare September 27, 2017 14:00

jburel added the exclude label Sep 29, 2017

jburel removed exclude ON_HOLD labels Oct 9, 2017

dominikl added 10 commits October 9, 2017 14:19

Remove usage of guest session

e9efd2b

Also throw DSOutOfServiceException on connection loss

4fe85ae

Add join session 'LoginCredentials' and test

287771d

Fix: Don't swallow Exception for Gateway.getSessionId()

3e636bc

Add accidentaly commented out tests again

acc7426

Use Java's UUID method instead of regex

9b8797d

Catch exception

88abb1e

Make Gateway AutoClosable

80342c1

Remove stale connectors

8c58265

Check arguments

a3915ee

dominikl force-pushed the java_gateway_fixes branch from f20bf97 to a3915ee Compare October 9, 2017 13:19

dominikl added 2 commits October 11, 2017 13:54

getHierarchy without ids is a valid request (~ 'get all')

edbf0cd

Just clarified doc for getHierarchy method

c094f0a

mtbc reviewed Oct 12, 2017

View reviewed changes

dominikl added 2 commits October 12, 2017 11:41

Removed printStackTrace from integration test

a1f1a73

Adjusted javadoc comment

84a45bf

joshmoore changed the base branch from develop to clisplit November 21, 2017 10:55

joshmoore merged commit 48d5860 into ome:clisplit Nov 23, 2017

joshmoore added this to the 5.4.2 milestone Jan 16, 2018

joshmoore mentioned this pull request Jan 16, 2018

Re-integrate next_infra into develop #5616

Merged

dominikl mentioned this pull request Feb 7, 2018

Insight bug fixes #5650

Merged

dominikl deleted the java_gateway_fixes branch April 16, 2018 12:32

chris-allan mentioned this pull request May 12, 2022

Add integration test for joining session #6319

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java gateway bugfixes #5513

Java gateway bugfixes #5513

dominikl commented Sep 18, 2017 •

edited

joshmoore Sep 21, 2017 •

edited

dominikl Sep 21, 2017

dominikl Sep 21, 2017

joshmoore Sep 21, 2017

joshmoore Sep 21, 2017

dominikl Sep 21, 2017

mtbc Oct 13, 2017

dominikl commented Sep 26, 2017

mtbc Oct 12, 2017

dominikl Oct 12, 2017

mtbc Oct 12, 2017

dominikl commented Oct 16, 2017

mtbc commented Oct 16, 2017

dominikl commented Oct 16, 2017

pwalczysko commented Oct 17, 2017

joshmoore commented Nov 23, 2017

Java gateway bugfixes #5513

Java gateway bugfixes #5513

Conversation

dominikl commented Sep 18, 2017 • edited

What this PR does

Testing this PR

Related reading

joshmoore Sep 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikl commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikl commented Oct 16, 2017

mtbc commented Oct 16, 2017

dominikl commented Oct 16, 2017

pwalczysko commented Oct 17, 2017

joshmoore commented Nov 23, 2017

dominikl commented Sep 18, 2017 •

edited

joshmoore Sep 21, 2017 •

edited