[session] cleanup session-time / cleanup-session-time...

which are hard to grasp while there is no actual benefit to handle both.

Now repo will close session without any activity since cleanup-session-time (24h by default), and the web authentication chain won't reconnect automatically anymore. I don't think there is a big deal in keeping repo session for such time.

Also, ask the repo for latest session usage time, we can't know it for real on the web side (think of long running transactions).

Closes #1083245.

authorSylvain Th?nault <sylvain.thenault@logilab.fr>
changesetd56fd78006cd
branchdefault
phasepublic
hiddenno
parent revision#b5f15098f282 [debug] when a loop is detected in a tree, log the entity involved in it to ease repair
child revision#8ca424bc393b [dbapi] cleanup shared data api: let access to transaction from dbapi, we can write it after all... Also, querydata is better named txdata
files modified by this revision
dbapi.py
etwist/twconfig.py
misc/migration/3.10.0_common.py
server/repository.py
server/serverconfig.py
web/application.py
web/views/authentication.py
web/views/debug.py
web/views/sessions.py
web/webconfig.py
# HG changeset patch
# User Sylvain Thénault <sylvain.thenault@logilab.fr>
# Date 1280138904 -7200
# Mon Jul 26 12:08:24 2010 +0200
# Node ID d56fd78006cd1888c26182a28b11c943622ad1c2
# Parent b5f15098f282216cb91a5c507c0dc49e074a7eae
[session] cleanup session-time / cleanup-session-time...

which are hard to grasp while there is no actual benefit to handle both.

Now repo will close session without any activity since cleanup-session-time (24h by default),
and the web authentication chain won't reconnect automatically anymore. I don't think there
is a big deal in keeping repo session for such time.

Also, ask the repo for latest session usage time, we can't know it for real on the
web side (think of long running transactions).

Closes #1083245.

diff --git a/dbapi.py b/dbapi.py
@@ -505,14 +505,16 @@
1 
2      def request(self):
3          return DBAPIRequest(self.vreg, DBAPISession(self))
4 
5      def check(self):
6 -        """raise `BadConnectionId` if the connection is no more valid"""
7 +        """raise `BadConnectionId` if the connection is no more valid, else
8 +        return its latest activity timestamp.
9 +        """
10          if self._closed is not None:
11              raise ProgrammingError('Closed connection')
12 -        self._repo.check_session(self.sessionid)
13 +        return self._repo.check_session(self.sessionid)
14 
15      def set_session_props(self, **props):
16          """raise `BadConnectionId` if the connection is no more valid"""
17          if self._closed is not None:
18              raise ProgrammingError('Closed connection')
diff --git a/etwist/twconfig.py b/etwist/twconfig.py
@@ -74,16 +74,10 @@
19            'default': None,
20            'help': 'if this option is set, use the specified user to start \
21  the repository rather than the user running the command',
22            'group': 'main', 'level': WebConfiguration.mode == 'system'
23            }),
24 -        ('session-time',
25 -         {'type' : 'time',
26 -          'default': '30min',
27 -          'help': 'session expiration time, default to 30 minutes',
28 -          'group': 'main', 'level': 1,
29 -          }),
30          ('pyro-server',
31           {'type' : 'yn',
32            # pyro is only a recommends by default, so don't activate it here
33            'default': False,
34            'help': 'run a pyro server',
diff --git a/misc/migration/3.10.0_common.py b/misc/migration/3.10.0_common.py
@@ -0,0 +1,1 @@
35 +option_group_changed('cleanup-session-time', 'web', 'main')
diff --git a/server/repository.py b/server/repository.py
@@ -265,11 +265,14 @@
36          if not (self.config.creating or self.config.repairing
37                  or self.config.quick_start):
38              # call instance level initialisation hooks
39              self.hm.call_hooks('server_startup', repo=self)
40              # register a task to cleanup expired session
41 -            self.looping_task(self.config['session-time']/3., self.clean_sessions)
42 +            self.cleanup_session_time = self.config['cleanup-session-time'] or 60 * 60 * 24
43 +            assert self.cleanup_session_time > 0
44 +            cleanup_session_interval = min(60*60, self.cleanup_session_time / 3)
45 +            self.looping_task(cleanup_session_interval, self.clean_sessions)
46          assert isinstance(self._looping_tasks, list), 'already started'
47          for i, (interval, func, args) in enumerate(self._looping_tasks):
48              self._looping_tasks[i] = task = utils.LoopTask(interval, func, args)
49              self.info('starting task %s with interval %.2fs', task.name,
50                        interval)
@@ -620,12 +623,14 @@
51              return self.type_and_source_from_eid(eid, session)
52          finally:
53              session.reset_pool()
54 
55      def check_session(self, sessionid):
56 -        """raise `BadConnectionId` if the connection is no more valid"""
57 -        self._get_session(sessionid, setpool=False)
58 +        """raise `BadConnectionId` if the connection is no more valid, else
59 +        return its latest activity timestamp.
60 +        """
61 +        return self._get_session(sessionid, setpool=False).timestamp
62 
63      def get_shared_data(self, sessionid, key, default=None, pop=False):
64          """return the session's data dictionary"""
65          session = self._get_session(sessionid, setpool=False)
66          return session.get_shared_data(key, default, pop)
@@ -769,11 +774,11 @@
67 
68      def clean_sessions(self):
69          """close sessions not used since an amount of time specified in the
70          configuration
71          """
72 -        mintime = time() - self.config['session-time']
73 +        mintime = time() - self.cleanup_session_time
74          self.debug('cleaning session unused since %s',
75                     strftime('%T', localtime(mintime)))
76          nbclosed = 0
77          for session in self._sessions.values():
78              if session.timestamp < mintime:
diff --git a/server/serverconfig.py b/server/serverconfig.py
@@ -118,14 +118,20 @@
79            'default': None,
80            'help': 'if this option is set, use the specified user to start \
81  the repository rather than the user running the command',
82            'group': 'main', 'level': (CubicWebConfiguration.mode == 'installed') and 0 or 1,
83            }),
84 -        ('session-time',
85 +        ('cleanup-session-time',
86           {'type' : 'time',
87 -          'default': '30min',
88 -          'help': 'session expiration time, default to 30 minutes',
89 +          'default': '24h',
90 +          'help': 'duration of inactivity after which a session '
91 +          'will be closed, to limit memory consumption (avoid sessions that '
92 +          'never expire and cause memory leak when http-session-time is 0, or '
93 +          'because of bad client that never closes their connection). '
94 +          'So notice that even if http-session-time is 0 and the user don\'t '
95 +          'close his browser, he will have to reauthenticate after this time '
96 +          'of inactivity. Default to 24h.',
97            'group': 'main', 'level': 3,
98            }),
99          ('connections-pool-size',
100           {'type' : 'int',
101            'default': 4,
diff --git a/web/application.py b/web/application.py
@@ -29,11 +29,11 @@
102  from rql import BadRQLQuery
103 
104  from cubicweb import set_log_methods, cwvreg
105  from cubicweb import (
106      ValidationError, Unauthorized, AuthenticationError, NoSelectableObject,
107 -    RepositoryError, CW_EVENT_MANAGER)
108 +    RepositoryError, BadConnectionId, CW_EVENT_MANAGER)
109  from cubicweb.dbapi import DBAPISession
110  from cubicweb.web import LOGGER, component
111  from cubicweb.web import (
112      StatusResponse, DirectResponse, Redirect, NotFound, LogOut,
113      RemoteCallFailed, InvalidSession, RequestError)
@@ -46,52 +46,47 @@
114      """manage session data associated to a session identifier"""
115      __regid__ = 'sessionmanager'
116 
117      def __init__(self, vreg):
118          self.session_time = vreg.config['http-session-time'] or None
119 -        if self.session_time is not None:
120 -            assert self.session_time > 0
121 -            self.cleanup_session_time = self.session_time
122 -        else:
123 -            self.cleanup_session_time = vreg.config['cleanup-session-time'] or 1440 * 60
124 -            assert self.cleanup_session_time > 0
125 -        self.cleanup_anon_session_time = vreg.config['cleanup-anonymous-session-time'] or 5 * 60
126 -        assert self.cleanup_anon_session_time > 0
127          self.authmanager = vreg['components'].select('authmanager', vreg=vreg)
128 +        interval = (self.session_time or 0) / 2.
129          if vreg.config.anonymous_user() is not None:
130 -            self.clean_sessions_interval = max(
131 -                5 * 60, min(self.cleanup_session_time / 2.,
132 -                            self.cleanup_anon_session_time / 2.))
133 -        else:
134 -            self.clean_sessions_interval = max(
135 -                5 * 60,
136 -                self.cleanup_session_time / 2.)
137 +            self.cleanup_anon_session_time = vreg.config['cleanup-anonymous-session-time'] or 5 * 60
138 +            assert self.cleanup_anon_session_time > 0
139 +            if self.session_time is not None:
140 +                self.cleanup_anon_session_time = min(self.session_time,
141 +                                                     self.cleanup_anon_session_time)
142 +            interval = self.cleanup_anon_session_time / 2.
143 +        # we don't want to check session more than once every 5 minutes
144 +        self.clean_sessions_interval = max(5 * 60, interval)
145 
146      def clean_sessions(self):
147          """cleanup sessions which has not been unused since a given amount of
148          time. Return the number of sessions which have been closed.
149          """
150          self.debug('cleaning http sessions')
151 +        session_time = self.session_time
152          closed, total = 0, 0
153          for session in self.current_sessions():
154 -            no_use_time = (time() - session.last_usage_time)
155              total += 1
156 -            if session.anonymous_session:
157 -                if no_use_time >= self.cleanup_anon_session_time:
158 +            try:
159 +                last_usage_time = session.cnx.check()
160 +            except BadConnectionId:
161 +                self.close_session(session)
162 +                closed += 1
163 +            else:
164 +                no_use_time = (time() - last_usage_time)
165 +                if session.anonymous_session:
166 +                    if no_use_time >= self.cleanup_anon_session_time:
167 +                        self.close_session(session)
168 +                        closed += 1
169 +                elif session_time is not None and no_use_time >= session_time:
170                      self.close_session(session)
171                      closed += 1
172 -            elif no_use_time >= self.cleanup_session_time:
173 -                self.close_session(session)
174 -                closed += 1
175          return closed, total - closed
176 
177 -    def has_expired(self, session):
178 -        """return True if the web session associated to the session is expired
179 -        """
180 -        return not (self.session_time is None or
181 -                    time() < session.last_usage_time + self.session_time)
182 -
183      def current_sessions(self):
184          """return currently open sessions"""
185          raise NotImplementedError()
186 
187      def get_session(self, req, sessionid):
@@ -211,23 +206,19 @@
188                  try:
189                      session = self.open_session(req)
190                  except AuthenticationError:
191                      req.remove_cookie(cookie, self.SESSION_VAR)
192                      raise
193 -        # remember last usage time for web session tracking
194 -        session.last_usage_time = time()
195 
196      def get_session(self, req, sessionid):
197          return self.session_manager.get_session(req, sessionid)
198 
199      def open_session(self, req):
200          session = self.session_manager.open_session(req)
201          cookie = req.get_cookie()
202          cookie[self.SESSION_VAR] = session.sessionid
203          req.set_cookie(cookie, self.SESSION_VAR, maxage=None)
204 -        # remember last usage time for web session tracking
205 -        session.last_usage_time = time()
206          if not session.anonymous_session:
207              self._postlogin(req)
208          return session
209 
210      def _update_last_login_time(self, req):
diff --git a/web/views/authentication.py b/web/views/authentication.py
@@ -72,11 +72,11 @@
211      def __init__(self, vreg):
212          super(RepositoryAuthenticationManager, self).__init__(vreg)
213          self.repo = vreg.config.repository(vreg)
214          self.log_queries = vreg.config['query-log-file']
215          self.authinforetreivers = sorted(vreg['webauth'].possible_objects(vreg),
216 -                                    key=lambda x: x.order)
217 +                                         key=lambda x: x.order)
218          # 2-uple login / password, login is None when no anonymous access
219          # configured
220          self.anoninfo = vreg.config.anonymous_user()
221          if self.anoninfo[0]:
222              self.anoninfo = (self.anoninfo[0], {'password': self.anoninfo[1]})
@@ -96,29 +96,15 @@
223          # email, login and cnx.login are the email while user.login is the
224          # actual user login
225          if login and session.login != login:
226              raise InvalidSession('login mismatch')
227          try:
228 -            lock = session.reconnection_lock
229 -        except AttributeError:
230 -            lock = session.reconnection_lock = Lock()
231 -        # need to be locked two avoid duplicated reconnections on concurrent
232 -        # requests
233 -        with lock:
234 -            cnx = session.cnx
235 -            try:
236 -                # calling cnx.user() check connection validity, raise
237 -                # BadConnectionId on failure
238 -                user = cnx.user(req)
239 -            except BadConnectionId:
240 -                # check if a connection should be automatically restablished
241 -                if (login is None or login == session.login):
242 -                    cnx = self._authenticate(session.login, session.authinfo)
243 -                    user = cnx.user(req)
244 -                    session.cnx = cnx
245 -                else:
246 -                    raise InvalidSession('bad connection id')
247 +            # calling cnx.user() check connection validity, raise
248 +            # BadConnectionId on failure
249 +            user = session.cnx.user(req)
250 +        except BadConnectionId:
251 +            raise InvalidSession('bad connection id')
252          return user
253 
254      def authenticate(self, req):
255          """authenticate user using connection information found in the request,
256          and return corresponding a :class:`~cubicweb.dbapi.Connection` instance,
diff --git a/web/views/debug.py b/web/views/debug.py
@@ -116,14 +116,19 @@
257          sessions = SESSION_MANAGER.current_sessions()
258          w(u'<h3>%s</h3>' % _('opened web sessions'))
259          if sessions:
260              w(u'<ul>')
261              for session in sessions:
262 +                try:
263 +                    last_usage_time = session.cnx.check()
264 +                except BadConnectionId:
265 +                    w(u'<li>%s (INVALID)</li>' % session.sessionid)
266 +                    continue
267                  w(u'<li>%s (%s: %s)<br/>' % (
268                      session.sessionid,
269                      _('last usage'),
270 -                    strftime(dtformat, localtime(session.last_usage_time))))
271 +                    strftime(dtformat, localtime(last_usage_time))))
272                  dict_to_html(w, session.data)
273                  w(u'</li>')
274              w(u'</ul>')
275          else:
276              w(u'<p>%s</p>' % _('no web sessions found'))
diff --git a/web/views/sessions.py b/web/views/sessions.py
@@ -15,12 +15,12 @@
277  #
278  # You should have received a copy of the GNU Lesser General Public License along
279  # with CubicWeb.  If not, see <http://www.gnu.org/licenses/>.
280  """web session component: by dfault the session is actually the db connection
281  object :/
282 +"""
283 
284 -"""
285  __docformat__ = "restructuredtext en"
286 
287  from cubicweb.web import InvalidSession
288  from cubicweb.web.application import AbstractSessionManager
289  from cubicweb.dbapi import DBAPISession
@@ -49,13 +49,10 @@
290      def get_session(self, req, sessionid):
291          """return existing session for the given session identifier"""
292          if not sessionid in self._sessions:
293              raise InvalidSession()
294          session = self._sessions[sessionid]
295 -        if self.has_expired(session):
296 -            self.close_session(session)
297 -            raise InvalidSession()
298          try:
299              user = self.authmanager.validate_session(req, session)
300          except InvalidSession:
301              # invalid session
302              self.close_session(session)
diff --git a/web/webconfig.py b/web/webconfig.py
@@ -133,21 +133,10 @@
303            'help': "duration of the cookie used to store session identifier. "
304            "If 0, the cookie will expire when the user exist its browser. "
305            "Should be 0 or greater than repository\'s session-time.",
306            'group': 'web', 'level': 2,
307            }),
308 -        ('cleanup-session-time',
309 -         {'type' : 'time',
310 -          'default': '24h',
311 -          'help': 'duration of inactivity after which a connection '
312 -          'will be closed, to limit memory consumption (avoid sessions that '
313 -          'never expire and cause memory leak when http-session-time is 0). '
314 -          'So even if http-session-time is 0 and the user don\'t close his '
315 -          'browser, he will have to reauthenticate after this time of '
316 -          'inactivity. Default to 24h.',
317 -          'group': 'web', 'level': 3,
318 -          }),
319          ('cleanup-anonymous-session-time',
320           {'type' : 'time',
321            'default': '5min',
322            'help': 'Same as cleanup-session-time but specific to anonymous '
323            'sessions. You can have a much smaller timeout here since it will be '