[repo] Stop closing session
authorSylvain Thénault <sylvain.thenault@logilab.fr>
Fri, 10 Mar 2017 18:24:40 +0100
changeset 12027 c62c80f20a82
parent 12026 c21b399c9269
child 12028 08c866d2f11d
[repo] Stop closing session The only point in "closing" session is to call the `session_close` event. Since I'm not aware of any application relying on it, I think this is too costly (in term of code) to maintain and propose to drop it.
cubicweb/devtools/__init__.py
cubicweb/devtools/fake.py
cubicweb/devtools/test/unittest_testlib.py
cubicweb/devtools/testlib.py
cubicweb/hooks/syncsession.py
cubicweb/server/hook.py
cubicweb/server/repository.py
cubicweb/server/session.py
cubicweb/server/test/data/hooks.py
cubicweb/server/test/unittest_hook.py
cubicweb/server/test/unittest_ldapsource.py
cubicweb/server/test/unittest_repository.py
cubicweb/server/test/unittest_security.py
cubicweb/web/views/sessions.py
--- a/cubicweb/devtools/__init__.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/devtools/__init__.py	Fri Mar 10 18:24:40 2017 +0100
@@ -108,7 +108,6 @@
     * system source is shutdown
     """
     if not repo._needs_refresh:
-        repo.close_sessions()
         for cnxset in repo.cnxsets:
             cnxset.close(True)
         repo.system_source.shutdown()
--- a/cubicweb/devtools/fake.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/devtools/fake.py	Fri Mar 10 18:24:40 2017 +0100
@@ -151,8 +151,7 @@
 
     def commit(self, *args):
         self.transaction_data.clear()
-    def close(self, *args):
-        pass
+
     def system_sql(self, sql, args=None):
         pass
 
--- a/cubicweb/devtools/test/unittest_testlib.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/devtools/test/unittest_testlib.py	Fri Mar 10 18:24:40 2017 +0100
@@ -285,10 +285,6 @@
             self.assertTrue(rset)
             self.assertEqual('babar', req.form['elephant'])
 
-    def test_close(self):
-        acc = self.new_access('admin')
-        acc.close()
-
     def test_admin_access(self):
         with self.admin_access.client_cnx() as cnx:
             self.assertEqual('admin', cnx.user.login)
--- a/cubicweb/devtools/testlib.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/devtools/testlib.py	Fri Mar 10 18:24:40 2017 +0100
@@ -223,11 +223,6 @@
 
     .. automethod:: cubicweb.testlib.RepoAccess.cnx
     .. automethod:: cubicweb.testlib.RepoAccess.web_request
-
-    The RepoAccess need to be closed to destroy the associated Session.
-    TestCase usually take care of this aspect for the user.
-
-    .. automethod:: cubicweb.testlib.RepoAccess.close
     """
 
     def __init__(self, repo, login, requestcls):
@@ -279,10 +274,6 @@
             req.set_cnx(cnx)
             yield req
 
-    def close(self):
-        """Close the session associated to the RepoAccess"""
-        self._session.close()
-
     @contextmanager
     def shell(self):
         from cubicweb.server.migractions import ServerMigrationHelper
@@ -360,7 +351,7 @@
     def _close_access(self):
         while self._open_access:
             try:
-                self._open_access.pop().close()
+                self._open_access.pop()
             except BadConnectionId:
                 continue  # already closed
 
@@ -458,7 +449,6 @@
     def tearDown(self):
         # XXX hack until logilab.common.testlib is fixed
         if self._admin_session is not None:
-            self._admin_session.close()
             self._admin_session = None
         while self._cleanups:
             cleanup, args, kwargs = self._cleanups.pop(-1)
--- a/cubicweb/hooks/syncsession.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/hooks/syncsession.py	Fri Mar 10 18:24:40 2017 +0100
@@ -116,7 +116,6 @@
             # remove cached groups for the user
             key = user_session_cache_key(self.session.user.eid, 'groups')
             self.session.data.pop(key, None)
-            self.session.close()
         except BadConnectionId:
             pass  # already closed
 
--- a/cubicweb/server/hook.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/server/hook.py	Fri Mar 10 18:24:40 2017 +0100
@@ -189,8 +189,7 @@
 `server_restore`) have a `repo` and a `timestamp` attributes, but
 *their `_cw` attribute is None*.
 
-Hooks called on session event (eg `session_open`, `session_close`) have no
-special attribute.
+Hooks called on session event (`session_open`) have no special attribute.
 
 
 API
@@ -273,7 +272,7 @@
 SYSTEM_HOOKS = set(('server_backup', 'server_restore',
                     'server_startup', 'server_maintenance',
                     'server_shutdown', 'before_server_shutdown',
-                    'session_open', 'session_close'))
+                    'session_open',))
 
 ALL_HOOKS = ENTITIES_HOOKS | RELATIONS_HOOKS | SYSTEM_HOOKS
 
--- a/cubicweb/server/repository.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/server/repository.py	Fri Mar 10 18:24:40 2017 +0100
@@ -458,7 +458,6 @@
             self.info('waiting thread %s...', thread.getName())
             thread.join()
             self.info('thread %s finished', thread.getName())
-        self.close_sessions()
         self.cnxsets.close()
         hits, misses = self.querier.cache_hit, self.querier.cache_miss
         try:
@@ -691,11 +690,6 @@
 
     # session handling ########################################################
 
-    def close_sessions(self):
-        """close every opened sessions"""
-        for session in list(self._sessions.values()):
-            session.close()
-
     def clean_sessions(self):
         """close sessions not used since an amount of time specified in the
         configuration
--- a/cubicweb/server/session.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/server/session.py	Fri Mar 10 18:24:40 2017 +0100
@@ -955,18 +955,6 @@
         self.repo = repo
         self._timestamp = Timestamp()
         self.data = {}
-        self.closed = False
-
-    def close(self):
-        if self.closed:
-            self.warning('closing already closed session %s', self.sessionid)
-            return
-        with self.new_cnx() as cnx:
-            self.repo.hm.call_hooks('session_close', cnx)
-            cnx.commit()
-            del self.repo._sessions[self.sessionid]
-        self.closed = True
-        self.info('closed session %s for user %s', self.sessionid, self.user.login)
 
     def __unicode__(self):
         return '<session %s (%s 0x%x)>' % (
--- a/cubicweb/server/test/data/hooks.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/server/test/data/hooks.py	Fri Mar 10 18:24:40 2017 +0100
@@ -40,9 +40,3 @@
     events = ('session_open',)
     def __call__(self):
         CALLED_EVENTS['session_open'] = self._cw.user.login
-
-class LogoutHook(Hook):
-    __regid__ = 'mylogout'
-    events = ('session_close',)
-    def __call__(self):
-        CALLED_EVENTS['session_close'] = self._cw.user.login
--- a/cubicweb/server/test/unittest_hook.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/server/test/unittest_hook.py	Fri Mar 10 18:24:40 2017 +0100
@@ -139,8 +139,6 @@
         anonaccess = self.new_access('anon')
         with anonaccess.repo_cnx() as cnx:
             self.assertEqual(hooks.CALLED_EVENTS['session_open'], 'anon')
-        anonaccess.close()
-        self.assertEqual(hooks.CALLED_EVENTS['session_close'], 'anon')
 
 
 if __name__ == '__main__':
--- a/cubicweb/server/test/unittest_ldapsource.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/server/test/unittest_ldapsource.py	Fri Mar 10 18:24:40 2017 +0100
@@ -246,9 +246,7 @@
             self.assertRaises(AuthenticationError,
                               source.authenticate, cnx, 'syt', 'toto')
             self.assertTrue(source.authenticate(cnx, 'syt', 'syt'))
-        session = self.repo.new_session('syt', password='syt')
-        self.assertTrue(session)
-        session.close()
+        self.assertTrue(self.repo.new_session('syt', password='syt'))
 
     def test_base(self):
         with self.admin_access.repo_cnx() as cnx:
@@ -420,9 +418,7 @@
             cnx.commit()
         # and that we can now authenticate again
         self.assertRaises(AuthenticationError, self.repo.new_session, 'syt', password='toto')
-        session = self.repo.new_session('syt', password='syt')
-        self.assertTrue(session)
-        session.close()
+        self.assertTrue(self.repo.new_session('syt', password='syt'))
 
 
 class LDAPFeedGroupTC(LDAPFeedTestBase):
--- a/cubicweb/server/test/unittest_repository.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/server/test/unittest_repository.py	Fri Mar 10 18:24:40 2017 +0100
@@ -77,9 +77,7 @@
             self.assertFalse(cnx.execute('Any X WHERE NOT X cw_source S'))
 
     def test_connect(self):
-        session = self.repo.new_session(self.admlogin, password=self.admpassword)
-        self.assertTrue(session.sessionid)
-        session.close()
+        self.assertTrue(self.repo.new_session(self.admlogin, password=self.admpassword))
         self.assertRaises(AuthenticationError,
                           self.repo.connect, self.admlogin, password='nimportnawak')
         self.assertRaises(AuthenticationError,
@@ -100,9 +98,7 @@
                         {'login': u"barnabé", 'passwd': u"héhéhé".encode('UTF8')})
             cnx.commit()
         repo = self.repo
-        session = repo.new_session(u"barnabé", password=u"héhéhé".encode('UTF8'))
-        self.assertTrue(session.sessionid)
-        session.close()
+        self.assertTrue(repo.new_session(u"barnabé", password=u"héhéhé".encode('UTF8')))
 
     def test_rollback_on_execute_validation_error(self):
         class ValidationErrorAfterHook(Hook):
@@ -142,14 +138,6 @@
                 cnx.rollback()
                 self.assertFalse(cnx.execute('Any X WHERE X is CWGroup, X name "toto"'))
 
-
-    def test_close(self):
-        repo = self.repo
-        session = repo.new_session(self.admlogin, password=self.admpassword)
-        self.assertTrue(session.sessionid)
-        session.close()
-
-
     def test_initial_schema(self):
         schema = self.repo.schema
         # check order of attributes is respected
@@ -198,7 +186,6 @@
         session = repo.new_session(self.admlogin, password=self.admpassword)
         with session.new_cnx() as cnx:
             self.assertEqual(repo.type_from_eid(2, cnx), 'CWGroup')
-        session.close()
 
     def test_public_api(self):
         self.assertEqual(self.repo.get_schema(), self.repo.schema)
--- a/cubicweb/server/test/unittest_security.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/server/test/unittest_security.py	Fri Mar 10 18:24:40 2017 +0100
@@ -86,14 +86,12 @@
                                      "WHERE cw_login = 'oldpassword'").fetchone()[0]
             oldhash = self.repo.system_source.binary_to_str(oldhash)
             session = self.repo.new_session('oldpassword', password='oldpassword')
-            session.close()
             newhash = cnx.system_sql("SELECT cw_upassword FROM cw_CWUser "
                                      "WHERE cw_login = 'oldpassword'").fetchone()[0]
             newhash = self.repo.system_source.binary_to_str(newhash)
             self.assertNotEqual(oldhash, newhash)
             self.assertTrue(newhash.startswith(b'$6$'))
             session = self.repo.new_session('oldpassword', password='oldpassword')
-            session.close()
             newnewhash = cnx.system_sql("SELECT cw_upassword FROM cw_CWUser WHERE "
                                         "cw_login = 'oldpassword'").fetchone()[0]
             newnewhash = self.repo.system_source.binary_to_str(newnewhash)
@@ -305,8 +303,7 @@
             cnx.execute('SET X upassword %(passwd)s WHERE X eid %(x)s',
                        {'x': ueid, 'passwd': b'newpwd'})
             cnx.commit()
-        session = self.repo.new_session('user', password='newpwd')
-        session.close()
+        self.repo.new_session('user', password='newpwd')
 
     def test_user_cant_change_other_upassword(self):
         with self.admin_access.repo_cnx() as cnx:
--- a/cubicweb/web/views/sessions.py	Fri Mar 10 12:07:29 2017 +0100
+++ b/cubicweb/web/views/sessions.py	Fri Mar 10 18:24:40 2017 +0100
@@ -127,9 +127,6 @@
         except InvalidSession:
             self.close_session(session)
             raise
-        if session.closed:
-            self.close_session(session)
-            raise InvalidSession()
         return session
 
     def open_session(self, req):
@@ -176,4 +173,3 @@
         """
         self.info('closing http session %s' % session.sessionid)
         self._sessions.pop(session.sessionid, None)
-        session.close()