Edgewall Software
Modify

Opened 10 years ago

Closed 8 years ago

#9453 closed defect (fixed)

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

Reported by: Jun Omae Owned by: Jun Omae
Priority: normal Milestone: 1.0
Component: general Version: 0.11
Severity: normal Keywords:
Cc: leho@…, Thijs Triemstra, osimons Branch:
Release Notes:

When modified, the session is now always saved regardless of the way the response is sent (except for send_error).

API Changes:
Internal Changes:

Description (last modified by Christian Boos)

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

    diff --git a/trac/web/api.py b/trac/web/api.py
    index df6d9ac..b3becbd 100644
    a b class Request(object):  
    341341        if (not inm or inm != etag):
    342342            self.send_header('ETag', etag)
    343343        else:
     344            self.session.save()
    344345            self.send_response(304)
    345346            self.send_header('Content-Length', 0)
    346347            self.end_headers()
    class Request(object):  
    402403        self.send(data, content_type, status)
    403404
    404405    def send(self, content, content_type='text/html', status=200):
     406        self.session.save()
    405407        self.send_response(status)
    406408        self.send_header('Cache-Control', 'must-revalidate')
    407409        self.send_header('Content-Type', content_type + ';charset=utf-8')
    class Request(object):  
    477479        mtime = datetime.fromtimestamp(stat.st_mtime, localtz)
    478480        last_modified = http_date(mtime)
    479481        if last_modified == self.get_header('If-Modified-Since'):
     482            self.session.save()
    480483            self.send_response(304)
    481484            self.send_header('Content-Length', 0)
    482485            self.end_headers()
    class Request(object):  
    486489            mimetype = mimetypes.guess_type(path)[0] or \
    487490                       'application/octet-stream'
    488491
     492        self.session.save()
    489493        self.send_response(200)
    490494        self.send_header('Content-Type', mimetype)
    491495        self.send_header('Content-Length', stat.st_size)

Attachments (0)

Change History (17)

comment:1 by Christian Boos, 10 years ago

Milestone: 0.13

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

  • trac/web/main.py

     
    260260                else:
    261261                    self._post_process_request(req)
    262262            except RequestDone:
     263                # Give the session a chance to persist changes after a send()
     264                req.session.save()
    263265                raise
    264266            except:
    265267                # post-process the request in case of errors

(untested)

comment:2 by osimons, 10 years ago

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).

comment:3 by lkraav <leho@…>, 10 years ago

Cc: leho@… added

comment:4 by Christian Boos, 10 years ago

Can someone please test this?

comment:5 by Christian Boos, 10 years ago

Owner: set to Christian Boos

comment:6 by Thijs Triemstra, 10 years ago

Cc: Thijs Triemstra added

comment:7 by Christian Boos, 10 years ago

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

in reply to:  7 ; comment:8 by Jun Omae, 10 years ago

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

    diff --git a/trac/web/api.py b/trac/web/api.py
    index f461a2c..b0dcdef 100644
    a b class Request(object):  
    446446        for listener in self.redirect_listeners:
    447447            listener(self, url, permanent)
    448448
    449         self.session.save() # has to be done before the redirect is sent
    450 
    451449        if permanent:
    452450            status = 301 # 'Moved Permanently'
    453451        elif self.method == 'POST':
  • trac/web/main.py

    diff --git a/trac/web/main.py b/trac/web/main.py
    index 4449717..1cfac67 100644
    a b class RequestDispatcher(Component):  
    204204
    205205                    output = chrome.render_template(req, template, data,
    206206                                                    content_type)
    207                     # Give the session a chance to persist changes
    208                     req.session.save()
    209207                    req.send(output, content_type or 'text/html')
    210208                else:
    211209                    self._post_process_request(req)
    212210            except RequestDone:
     211                # Give the session a chance to persist changes after a send()
     212                req.session.save()
    213213                raise
    214214            except:
    215215                # post-process the request in case of errors

in reply to:  8 comment:9 by Jun Omae, 10 years ago

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.

comment:10 by osimons, 9 years ago

Cc: osimons added

comment:11 by osimons, 9 years ago

This looks good to commit IMHO. Jun?

comment:12 by Stefan, 9 years ago

Version: 0.120.11

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

comment:13 by Jun Omae, 9 years ago

Resolution: fixed
Status: newclosed

Thanks for the confirmation, Simon.

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

comment:14 by Jun Omae, 9 years ago

Owner: changed from Christian Boos to Jun Omae

comment:15 by Christian Boos, 8 years ago

Description: modified (diff)
Release Notes: modified (diff)
Summary: [PATCH] The session does not be saved with `req.send()`[PATCH] The session does not get saved with `req.send()`

comment:16 by Christian Boos, 8 years ago

Resolution: fixed
Status: closedreopened

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).

comment:17 by Remy Blank, 8 years ago

Resolution: fixed
Status: reopenedclosed

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.