[PATCH] The session does not get saved with `req.send()`

[Jun Omae]

When the session is modified and the response is sent with req.send(), the session does not get saved.

The patch adds self.session.save() to check_modified, send and send_file, excluding send_error.

Trac 0.12 and Trac 0.11-stable have the same issue.

trac/web/api.py

@@ -341 +341 @@
         if (not inm or inm != etag):
             self.send_header('ETag', etag)
         else:
+            self.session.save()
             self.send_response(304)
             self.send_header('Content-Length', 0)
             self.end_headers()
@@ -402 +403 @@
         self.send(data, content_type, status)
 
     def send(self, content, content_type='text/html', status=200):
+        self.session.save()
         self.send_response(status)
         self.send_header('Cache-Control', 'must-revalidate')
         self.send_header('Content-Type', content_type + ';charset=utf-8')
@@ -477 +479 @@
         mtime = datetime.fromtimestamp(stat.st_mtime, localtz)
         last_modified = http_date(mtime)
         if last_modified == self.get_header('If-Modified-Since'):
+            self.session.save()
             self.send_response(304)
             self.send_header('Content-Length', 0)
             self.end_headers()
@@ -486 +489 @@
             mimetype = mimetypes.guess_type(path)[0] or \
                        'application/octet-stream'
 
+        self.session.save()
         self.send_response(200)
         self.send_header('Content-Type', mimetype)
         self.send_header('Content-Length', stat.st_size)

[Christian Boos]

As one is supposed to raise RequestDone after a send, what about simply:

trac/web/main.py

@@ -260 +260 @@
                 else:
                     self._post_process_request(req)
             except RequestDone:
+                # Give the session a chance to persist changes after a send()
+                req.session.save()
                 raise
             except:
                 # post-process the request in case of errors

(untested)

[osimons]

Replying to cboos:

As one is supposed to raise RequestDone after a send, what about simply:…

Yes, that makes sense as a general solution as plugins may implement similar logic too - like I've done for ​bitten:source:trunk/bitten/master.py (see BuildMaster._send_response() currently at line 139).

[Christian Boos]

Can someone please test this?

[Christian Boos]

Jun, for what kind of requests do we need to save the session after a send()?

[Jun Omae]

Replying to cboos:

Jun, for what kind of requests do we need to save the session after a send()?

Well…, when Request.redirect() is called, the method saves a session before raising RequestDone. In the other methods which raises RequestDone (e.g. send()), the methods don't save it. I thought that the other methods should save it.

Your patch is simple and generic. In this case, I think that the saving a session in Request.redirect should be removed cause it saves a session, twice.

trac/web/api.py

@@ -446 +446 @@
         for listener in self.redirect_listeners:
             listener(self, url, permanent)
 
-        self.session.save() # has to be done before the redirect is sent
-
         if permanent:
             status = 301 # 'Moved Permanently'
         elif self.method == 'POST':

trac/web/main.py

@@ -204 +204 @@
 
                     output = chrome.render_template(req, template, data,
                                                     content_type)
-                    # Give the session a chance to persist changes
-                    req.session.save()
                     req.send(output, content_type or 'text/html')
                 else:
                     self._post_process_request(req)
             except RequestDone:
+                # Give the session a chance to persist changes after a send()
+                req.session.save()
                 raise
             except:
                 # post-process the request in case of errors

[Jun Omae]

Replying to jomae:

Your patch is simple and generic. In this case, I think that the saving a session in Request.redirect should be removed cause it saves a session, twice.

#!diff...

And this patch is tested.

[osimons]

This looks good to commit IMHO. Jun?

[Stefan]

According to the initial description the issue persisted since 0.11 at least already.

[Jun Omae]

Thanks for the confirmation, Simon.

The patch of comment:8 is committed in [10902].

[Christian Boos]

The initial approach was better, as doing the session.save() at the level of the RequestDone exception handler means that the request has been fully sent. The client might already have sent another query which gets processed (by another thread or process) before the save is complete (like the error in ticket:10779#comment:59).

[Remy Blank]

Fixed in [11236] by saving the session in send_response() instead of after sending the content.