MoinMoin: attachment:trail.patch of MoinMoinBugs/ConcurrencyAndFiles

   1 * looking for nirs@freeshell.org--2005/moin--fix--1.3--patch-23 to compare with
   2 * comparing to nirs@freeshell.org--2005/moin--fix--1.3--patch-23
   3 M  MoinMoin/user.py
   4 
   5 * modified files
   6 
   7 --- orig/MoinMoin/user.py
   8 +++ mod/MoinMoin/user.py
   9 @@ -6,7 +6,7 @@
  10      @license: GNU GPL, see COPYING for details.
  11  """
  12  
  13 -import os, string, time, Cookie, sha, codecs
  14 +import os, time, Cookie, sha, codecs
  15  
  16  try:
  17      import cPickle as pickle
  18 @@ -259,7 +259,8 @@
  19          
  20          # attrs not saved to profile
  21          self._request = request
  22 -        self._trail = []
  23 +        # This means trail is not known yet, must read from disk
  24 +        self._trail = None
  25  
  26          # create checkbox fields (with default 0)
  27          for key, label in self._checkbox_fields:
  28 @@ -410,8 +411,8 @@
  29          if -24 <= self.tz_offset and self.tz_offset <= 24:
  30              self.tz_offset = self.tz_offset * 3600
  31  
  32 -        # clear trail
  33 -        self._trail = []
  34 +        # clear trail, must read from disk
  35 +        self._trail = None
  36  
  37          if not self.disabled:
  38              self.valid = 1
  39 @@ -686,47 +687,63 @@
  40                  return 1
  41          return 0
  42  
  43 +    def shouldAddTrail(self, pageName):
  44 +        """ Return true if name should be added to trail """
  45 +        if not self.valid:
  46 +            return False
  47 +
  48 +        # Don't update if trail is not used currently.
  49 +        # TODO: This cause a little problem when activating 'show page
  50 +        # trail' since its starts with empty trail. If we remove this we
  51 +        # will have to pay one disk access per request for valid users.
  52 +        if not (self.show_page_trail or self.remember_last_visit):
  53 +            return False
  54 +        
  55 +        # Don't add missing pages or those the user may not read
  56 +        if self._request:
  57 +            from MoinMoin.Page import Page
  58 +            page = Page(self._request, pageName)
  59 +            if not (page.exists() and
  60 +                    self._request.user.may.read(page.page_name)):
  61 +                return False
  62 +        return True
  63  
  64      def addTrail(self, pagename):
  65 -        """
  66 -        Add page to trail.
  67 +        """ Add page to trail.
  68          
  69          @param pagename: the page name to add to the trail
  70          """
  71 -        # TODO: aquire lock here, so multiple processes don't clober
  72 -        # each one trail.
  73 -
  74 -        if self.valid and (self.show_page_trail or self.remember_last_visit):
  75 -            # load trail if not known
  76 -            self.getTrail()      
  77 -            
  78 -            # don't append tail to trail ;)
  79 -            if self._trail and self._trail[-1] == pagename: return
  80 +        if self.shouldAddTrail(pagename):
  81 +            # TODO: aquire lock so another process may not read or write
  82 +            # trail until we release the lock.
  83 +
  84 +            # Becuase one user can use multiple browsers, and each
  85 +            # session might add pages to the trail, we must read trail
  86 +            # from disk before adding items to the trail.
  87 +            self._loadTrail()      
  88 +
  89 +            # Don't append tail to trail
  90 +            if not pagename in self._trail[-1:]:
  91 +                # Remove pagename duplicates (from multiple sessions?)
  92 +                self._trail = [name for name in self._trail
  93 +                               if name != pagename]
  94 +                self._trail.append(pagename)
  95 +                self._trail = self._trail[-self._cfg.trail_size:]
  96 +                self._saveTrail()
  97  
  98 -            # Add only existing pages that the user may read
  99 -            if self._request:
 100 -                from MoinMoin.Page import Page
 101 -                page = Page(self._request, pagename)
 102 -                if not (page.exists() and
 103 -                        self._request.user.may.read(page.page_name)):
 104 -                    return
 105 -
 106 -            # append new page, limiting the length
 107 -            self._trail = filter(lambda p, pn=pagename: p != pn, self._trail)
 108 -            self._trail = self._trail[-(self._cfg.trail_size-1):]
 109 -            self._trail.append(pagename)
 110 -            self.saveTrail()
 111 +            # TODO: release lock
 112  
 113 -            ## TODO: release lock here
 114 -            
 115 -    def saveTrail(self):
 116 +    def _trailPath(self):
 117 +        return self.__filename() + ".trail"
 118 +        
 119 +    def _saveTrail(self):
 120          """ Save trail file
 121  
 122          Save using one write call, which should be fine in most cases,
 123          but will fail in rare cases without real file locking.
 124          """
 125          data = '\n'.join(self._trail) + '\n'
 126 -        path = self.__filename() + ".trail"
 127 +        path = self._trailPath()
 128          try:
 129              file = codecs.open(path, "w", config.charset)
 130              try:
 131 @@ -745,22 +762,45 @@
 132              self._request.log("Can't save trail file: %s" % str(err))
 133  
 134      def getTrail(self):
 135 -        """
 136 -        Return list of recently visited pages.
 137 +        """ Return list of recently visited pages.
 138 +
 139 +        Get the trail from disk once per user life span, which is one
 140 +        request. Later return cached trail.
 141          
 142          @rtype: list
 143          @return: pages in trail
 144          """
 145 -        if self.valid and (self.show_page_trail or self.remember_last_visit) \
 146 -                and not self._trail \
 147 -                and os.path.exists(self.__filename() + ".trail"):
 148 -            try:
 149 -                self._trail = codecs.open(self.__filename() + ".trail", 'r', config.charset).readlines()
 150 -            except (OSError, ValueError):
 151 -                self._trail = []
 152 +        if self._trail is None:
 153 +            # Initialize trail from disk if used, or to empty list
 154 +            if (self.valid and
 155 +                (self.show_page_trail or self.remember_last_visit)):
 156 +                self._loadTrail()
 157              else:
 158 -                self._trail = filter(None, map(string.strip, self._trail))
 159 -                self._trail = self._trail[-self._cfg.trail_size:]
 160 -
 161 +                self._trail = []  
 162          return self._trail
 163  
 164 +    def _loadTrail(self):
 165 +        """ Load trail from disk or set to empty list
 166 +
 167 +        Should be safe for normal use, even safer if we add file
 168 +        locking.
 169 +        """
 170 +        path = self._trailPath()
 171 +        # TODO: aquire lock
 172 +        try:
 173 +            file = codecs.open(path, 'r', config.charset)
 174 +            try:
 175 +                trail = file.readlines()
 176 +            finally:
 177 +                file.close()
 178 +            trail = [name.strip() for name in trail
 179 +                     if name != '' and not name.isspace()]
 180 +            self._trail = trail[-self._cfg.trail_size:]
 181 +        except (IOError, OSError, ValueError), err:
 182 +            # Don't log this because its ok if the trail was deleted,
 183 +            # its a problem only if we can't create it later.
 184 +            self._trail = []
 185 +        # TODO: release lock
 186 +            
 187 +                    
 188 +                
 189