Changeset 2597

Timestamp:

07/05/08 22:16:06 (3 months ago)

Author:

jmoore

Message:

ticket:1018 - Fixing memory leak in solution

• Added ome/services/messaging.xml to publish messages to child contexts
  
• Fixed ordering issues SessionManagerI
  
• Also:
  
  • Using ice_getCommunicator() if ic null in omero.client
    
  • Catching exceptions on session close in test.integration.library
    

Location:

trunk/components

Files:

• blitz/resources/beanRefContext.xml (modified) (2 diffs)
• blitz/resources/omero/test.xml (modified) (2 diffs)
• blitz/src/ome/services/blitz/fire/SessionManagerI.java (modified) (3 diffs)
• blitz/src/ome/services/blitz/impl/ServiceFactoryI.java (modified) (5 diffs)
• blitz/test/ome/icy/model/utests/ServiceFactoryServiceCreationDestructionTest.java (modified) (1 diff)
• server/resources/beanRefContext.xml (modified) (2 diffs)
• server/src/ome/services/sessions/SessionManager.java (modified) (1 diff)
• tools/OmeroPy/src/omero/__init__.py (modified) (1 diff)
• tools/OmeroPy/test/integration/isession.py (modified) (3 diffs)
• tools/OmeroPy/test/integration/library.py (modified) (3 diffs)

trunk/components/blitz/resources/beanRefContext.xml

@@ -24 +24 @@
         <constructor-arg index="0">
       <list>
+        <value>classpath:ome/services/messaging.xml</value>
         <value>classpath:omero/server.xml</value>
         <value>classpath:omero/constants.xml</value>
@@ -55 +56 @@
         <constructor-arg index="0">
       <list>
+        <value>classpath:ome/services/messaging.xml</value>
         <value>classpath:omero/server.xml</value>
         <value>classpath:omero/constants.xml</value>

trunk/components/blitz/resources/omero/test.xml

@@ -37 +37 @@
  <bean id="methodSecurity" factory-bean="methodMock" factory-method="proxy"/>
  <bean id="sessionManager" factory-bean="sessionsMock" factory-method="proxy"/>
- 
+ <bean id="executor"       factory-bean="executorMock" factory-method="proxy"/>
  <!-- Here it is necessary to specify the HardWiredInterceptors instances
  even if they are *not* specified in Spring -->
@@ -77 +77 @@
  </bean>
 
+ 
+  <bean id="executorMock" class="ome.icy.fixtures.ExecutorMockFactory"/>
+
 </beans>

trunk/components/blitz/src/ome/services/blitz/fire/SessionManagerI.java

@@ -128 +128 @@
                 log.debug(String.format("Created session %s for user %s",
                         id.name, userId));
-            } else {
-                sessionToClientIds.get(s.getUuid()).add(session.clientId);
-                if (log.isDebugEnabled()) {
-                    log.debug(String.format("Rejoined session %s for user %s",
-                            id.name, userId));
-                }
-
-            }
+            }
+            sessionToClientIds.get(s.getUuid()).add(session.clientId);
+            if (log.isDebugEnabled()) {
+                log.debug(String.format("Rejoined session %s for user %s",
+                        id.name, userId));
+            }
+
             return Glacier2.SessionPrxHelper.uncheckedCast(_prx);
 
         } catch (Exception t) {
+
+            if (t instanceof ApiUsageException) {
+                ApiUsageException aue = (ApiUsageException) t;
+                throw new CannotCreateSessionException(aue.message);
+            }
+
             ConvertToBlitzExceptionMessage convert = new ConvertToBlitzExceptionMessage(
                     this, t);
@@ -150 +155 @@
             if (convert.to instanceof CannotCreateSessionException) {
                 throw (CannotCreateSessionException) convert.to;
-            } else if (convert.to instanceof ApiUsageException) {
-                ApiUsageException aue = (ApiUsageException) convert.to;
-                throw new CannotCreateSessionException(aue.message);
             }
 
@@ -207 +209 @@
      *            {@link Ice.ObjectAdapter}
      */
-    private void reapSessions(Ice.Current cantUseThisCurrent) {
+    public void reapSessions(Ice.Current cantUseThisCurrent) {
         Ice.ObjectAdapter adapter = cantUseThisCurrent.adapter;
         synchronized (sessionsForReaping) {
-            List<String> ids = new ArrayList<String>(sessionsForReaping);
-            for (String id : ids) {
-                for (String clientId : sessionToClientIds.get(id)) {
+            List<String> sessionIds = new ArrayList<String>(sessionsForReaping);
+            for (String sessionId : sessionIds) {
+                Set<String> clientIds = sessionToClientIds.keySet();
+                for (String clientId : clientIds) {
                     try {
                         Ice.Identity iid = ServiceFactoryI.sessionId(clientId,
-                                id);
+                                sessionId);
                         Ice.Object obj = adapter.find(iid);
                         if (obj == null) {
-                            log.debug(id + " already removed.");
+                            log.debug(Ice.Util.identityToString(iid)
+                                    + " already removed.");
                         } else {
                             ServiceFactoryI sf = (ServiceFactoryI) obj;
                             sf.doDestroy(adapter);
                         }
-                        sessionsForReaping.remove(id);
                     } catch (Exception e) {
-                        log.error("Error reaping session " + id
+                        log.error("Error reaping session " + sessionId
                                 + " from client " + clientId, e);
                     }
                 }
+                sessionToClientIds.remove(sessionId);
+                sessionsForReaping.remove(sessionId);
             }
         }

trunk/components/blitz/src/ome/services/blitz/impl/ServiceFactoryI.java

@@ -21 +21 @@
 import ome.api.JobHandle;
 import ome.api.ServiceInterface;
-import ome.conditions.InternalException;
 import ome.logic.HardWiredInterceptor;
 import ome.services.blitz.fire.AopContextInitializer;
@@ -516 +515 @@
 
         }
+
+        // All resources cleaned up or not based on the reference count.
+        // Now we can remove the current session. If an exception if thrown,
+        // there's not much we can do.
+        try {
+            current.adapter.remove(sessionId(principal.getName()));
+        } catch (Throwable t) {
+            // FIXME
+            log.error("Possible memory leak: can't remove service factory", t);
+        }
+
     }
 
     /**
-     * Performs the actual cleanup operation. Since {@link #destroy()} is called
-     * non-discriminantly by the router, even when a client has just died, we
-     * have this internal method for handling the actual closing of resources.
+     * Performs the actual cleanup operation on all the resources shared between
+     * this and other {@link ServiceFactoryI} instances in the same
+     * {@link Session}. Since {@link #destroy()} is called regardless by the
+     * router, even when a client has just died, we have this internal method
+     * for handling the actual closing of resources.
      */
     public void doDestroy(Ice.ObjectAdapter adapter) {
@@ -534 +546 @@
         Set<String> ipIds = interactiveSlots.keySet();
         for (String id : ipIds) {
-            InteractiveProcessorI ip = (InteractiveProcessorI) interactiveSlots
-                    .get(id);
-            ip.stop();
-            adapter.remove(Ice.Util.stringToIdentity(id));
-            interactiveSlots.remove(id);
+            InteractiveProcessorI ip = null;
+            try {
+                ip = (InteractiveProcessorI) interactiveSlots.get(id);
+                ip.stop();
+            } catch (Exception e) {
+                log.error("Error stopping interactive processor: " + ip);
+            } finally {
+                adapter.remove(Ice.Util.stringToIdentity(id));
+                interactiveSlots.remove(id);
+            }
         }
 
@@ -568 +585 @@
         }
 
-        // All resources cleaned up to the best of our ability,
-        // now we can remove the current session. If an exception if thrown,
-        // there's not much we can do.
-        try {
-            adapter.remove(sessionId(principal.getName()));
-        } catch (Throwable t) {
-            // FIXME
-            InternalException ie = new InternalException(t.getMessage());
-            ie.setStackTrace(t.getStackTrace());
-        }
     }
 
@@ -710 +717 @@
             if (null == already) {
                 current.adapter.add(servant, id);
+                if (log.isInfoEnabled()) {
+                    log.info("Created servant:" + servantString(id, servant));
+                }
             } else {
                 current.adapter.remove(id);
                 current.adapter.add(servant, id);
-            }
-            if (log.isInfoEnabled()) {
-                log.info("Created servant:" + servantString(id, servant));
+                if (already.hashCode() != servant.hashCode()) {
+                    log.info(String.format("Replacing %s with %s",
+                            servantString(id, already), servantString(id,
+                                    servant)));
+                }
             }
         } catch (Exception e) {

trunk/components/blitz/test/ome/icy/model/utests/ServiceFactoryServiceCreationDestructionTest.java

@@ -165 +165 @@
         callsActiveServices(Collections.singletonList(reServiceId));
         String id = sf.activeServices(curr).get(0).toString();
-        Ice.Current curr = new Ice.Current();
-        curr.id = Ice.Util.stringToIdentity("username/sessionid");
-        curr.adapter = adapter;
         // Events now called by SessionManagerI
-        sf.unregisterServant(Ice.Util.stringToIdentity(id), curr);
+        sf.unregisterServant(Ice.Util.stringToIdentity(id), adapter);
     }
 

trunk/components/server/resources/beanRefContext.xml

@@ -24 +24 @@
     <constructor-arg index="0">
       <list>
+        <value>classpath:ome/services/messaging.xml</value>
         <value>classpath:ome/model.xml</value>
       </list>
@@ -34 +35 @@
     <constructor-arg index="0">
       <list>
+        <value>classpath:ome/services/messaging.xml</value>
         <!-- Arranged roughly in stack order -->
         <value>classpath:ome/services/config-local.xml</value>

trunk/components/server/src/ome/services/sessions/SessionManager.java

@@ -70 +70 @@
 
     /**
+     * Return the number of client which are presumed to be attached to this
+     * session.
+     */
+    int getReferenceCount(String uuid);
+
+    /**
      * @param sessionId
      * @return A current session. Null if the session id is not found.

trunk/components/tools/OmeroPy/src/omero/__init__.py

@@ -219 +219 @@
         """
 
-        # If 'sf' exists we remove it.
+        # If 'sf' exists we remove it, but save it for the weird chance that ic is None
+        sf = None
         if hasattr(self, 'sf'):
+            sf = self.sf
             self.sf = None
 
         # If 'ic' does not exist we don't have anything to do
         if not hasattr(self, 'ic') or not self.ic:
-            return
+            if sf:
+                self.ic = sf.ice_getCommunicator()
+            else:
+                return
 
         try:

trunk/components/tools/OmeroPy/test/integration/isession.py

@@ -9 +9 @@
 
 """
+import unittest
 import test.integration.library as lib
 import omero
 import omero_RTypes_ice
+import omero_Constants_ice
 from omero_model_PixelsI import PixelsI
 from omero_model_ImageI import ImageI
@@ -44 +46 @@
 
     def testCreateSessionForUser(self):
-        user = self.root.sf.getQueryService().findAllByQuery("""
+        p = omero.sys.Parameters()
+        p.theFilter = omero.sys.Filter()
+        p.theFilter.limit = omero.RInt(1)
+        user = self.root.sf.getQueryService().findByQuery("""
             select e from Experimenter e where e.id > 0 and e.omeName != 'guest'
-            """, None)[0]
+            """, p)
         p = omero.sys.Principal()
         p.name  = user.omeName.val
@@ -71 +76 @@
 
     def testCreationDestructionClosing(self):
-        self.client.detachOnDestroy()
-        sess = self.client.sf.ice_getIdentity().category
-        self.client.destroyConnection()
+        c1 = omero.client()
+        s1 = c1.createSession()
+        s1.detachOnDestroy()
+        uuid = s1.ice_getIdentity().name
 
-        self.client = omero.client()
-        self.client.createSession(sess, sess)
-        self.client.closeOnDestroy()
-        self.client.destroyConnection()
+        # Intermediate "disrupter"
+        c2 = omero.client()
+        s2 = c2.createSession(uuid, uuid)
+        s2.closeOnDestroy()
+        s2.getAdminService().getEventContext()
+        c2.closeSession()
+
+        # 1 should still be able to continue
+        s1.getAdminService().getEventContext()
+
+        # Now if s1 exists another session should be able to connect
+        c1.closeSession()
+        c3 = omero.client()
+        s3 = c3.createSession(uuid, uuid)
+        s3.closeOnDestroy()
+        s3.getAdminService().getEventContext()
+        c3.closeSession()
+
+        # Now a connection should not be possible
+        c4 = omero.client()
+        s4 = c4.createSession(uuid, uuid);
+
+    def testSimpleDestruction(self):
+        c = omero.client()
+        c.ic.getImplicitContext().put(omero.constants.CLIENTUUID,"SimpleDestruction")
+        s = c.createSession()
+        s.closeOnDestroy()
+        c.closeSession()
 
 if __name__ == '__main__':

trunk/components/tools/OmeroPy/test/integration/library.py

@@ -12 +12 @@
 import omero
 import tempfile
+import traceback
+import exceptions
 
 class ITest(unittest.TestCase):
@@ -32 +34 @@
 
     def tearDown(self):
-        self.client.closeSession()
+        failure = False
+        try:
+            self.client.closeSession()
+        except:
+            traceback.print_exc()
+            failure = True
         if self.root:
-            self.root.closeSession()
+            try:
+                self.root.closeSession()
+            except:
+                traceback.print_exc()
+                failure = True
         for tmpfile in self.tmpfiles:
             try:
@@ -40 +51 @@
             except:
                 print "Error closing:"+tmpfile
+        if failure:
+           raise exceptions.Exception("Exception on client.closeSession")