JSAMP HUB waits for callee before returning to caller

[Laurent Bourgès]

Mark,

here are the results of my intensive test session today:

> Just surprised by the connection listener that is not called during
> > shutdown, but it seems normal:
> > - HubConnection.unregister() called => hub
> > - GuiConnector (GUI wrapper on low-level hub connection) not notified =>
> no
> > listener called
>
> Does this mean that some clients fail to unregister when the
> JVM hosting them is shut down?
>

No all clients are well unregistered from the hub's point of view.
I only noticed that the ConnectionListener (unregister) was not called
during the shutdown hook. It is "normal" as I suspect that EDT calls are
disabled or callbacks not executed during the JVM shutdown: forget it !

>
> > I enabled the java console tracing to get javaws exceptions and there is
> > still one related to WebHubProfile.stop():
> >
> > 14:26:21.650 INFO  [AWT-EventQueue-2] fr.jmmc - Exiting with status code
> > '0'.
> > WARNING: Shutdown hook failure: java.lang.IllegalStateException: zip file
> > closed
> > java.lang.IllegalStateException: zip file closed
> ...
> >
> > To fix it, I modified this class to preload the Runnable class invoked in
> > the stop() method.
>
> I have incorporated basically this fix with some stylistic alterations -
> commit 0408cb8 on branch async-call.  Can you confirm that this works
> properly for you?
>

Yes, fixed.


> Good job Laurent!  thanks a lot for the tests, I feel fairly confident
> that it's OK now, I know that you're thorough!
>

Today I performed few crash tests: 1 hub running while many clients
(SampClient) are looping on connect / exit operations to let the JVM
shutdown hook performs the client unregistration: it works very well (from
2 up to 20 parallel shells) !

Thanks to this crash test, I encountered few NullPointerException in
CallableClientServer:

WARNING: Shutdown hook failure: java.lang.NullPointerException
java.lang.NullPointerException
    at
org.astrogrid.samp.xmlrpc.CallableClientServer.removeClient(CallableClientServer.java:67)
    at
org.astrogrid.samp.xmlrpc.CallableClientServer$1.removeClient(CallableClientServer.java:113)
    at
org.astrogrid.samp.xmlrpc.StandardHubConnection.unregister(StandardHubConnection.java:58)
    at
org.astrogrid.samp.xmlrpc.XmlRpcHubConnection.finish(XmlRpcHubConnection.java:198)
    at
org.astrogrid.samp.xmlrpc.XmlRpcHubConnection.access$000(XmlRpcHubConnection.java:30)
    at
org.astrogrid.samp.xmlrpc.XmlRpcHubConnection$1.run(XmlRpcHubConnection.java:65)
    at org.astrogrid.samp.ShutdownManager.doCleanup(ShutdownManager.java:92)
    at
org.astrogrid.samp.ShutdownManager.access$000(ShutdownManager.java:16)
    at org.astrogrid.samp.ShutdownManager$1.run(ShutdownManager.java:44)

Here is a patch:
# This patch file was generated by NetBeans IDE
# It uses platform neutral UTF-8 encoding and \n newlines.
--- HEAD
+++ Modified In Working Tree
@@ -61,18 +61,23 @@
     /**
      * Removes a CallableClient object from this server.
      *
-     * @param  privateKey   hub connection for which this client was added
+     * @param   connection  hub connection for the registered client on
behalf
+     *          of which the client will operate
      */
     public void removeClient( HubConnection connection ) {
+        if ( clientHandler_ != null) {
         clientHandler_.removeClient( connection );
     }
+    }

     /**
      * Tidies up resources.  Following a call to this method, no further
      * clients can be added.
      */
     public void close() {
+        if ( server_ != null ) {
         server_.removeHandler( clientHandler_ );
+        }
         server_ = null;
         clientHandler_ = null;
     }


>
> > PS: I think the documentation related to multi-threading in the SAMP
> > protocol & implementations could be improved as there is two API levels:
> > - low-level: creates one SAMP threads per message and can lead to
> > concurrency issues (take care)
> > - GUI level (Java): using the Event Dispatcher Thread (single thread)
> > ensures Swing components are consistent and thread-safe
> >
> > But in JSamp, there is so many features (hub, profiles, connections) that
> > it is not always obvious which threading model is in use (low-level or
> GUI)
> > ie single (thread-safe) / multiple threads and if it is a blocking / non
> > blocking behaviour.
>
> Fair comment - in my defense I'm following the practice of the J2SE
> which generally assumes that you know which methods have to be called
> on the EDT rather than actually telling you about it.
> The rules (I think) are similar here: things that have to do with Swing
> ought to be called on the EDT.  But if I get a load of free time I'll
> consider adding some text (or even @Annotations) to make this explicit.
>

OK. Maybe you should insist on the MessageHandler which is called by a SAMP
thread and the developper must:
- use the EDT via SwingUtilities.invokeLater(Runnable) to update any Swing
component and avoid concurrency issues (model)
- use SwingWorker pattern to process the message (long task) and then
update the GUI (Java 6+)

Moreover, I am a bit lost in the JSamp class hierarchy to know exactly
which feature (or side) is implemented (client, server, hub, connections,
rpc) ... and which thread executes them (EDT, samp thread)

Cheers,
Laurent
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.ivoa.net/pipermail/apps-samp/attachments/20140704/c386a602/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CallableClientServer.java
Type: text/x-java
Size: 4407 bytes
Desc: not available
URL: <http://www.ivoa.net/pipermail/apps-samp/attachments/20140704/c386a602/attachment.bin>