Hi Dieter, *,

Dieter Maurer wrote:

>   The "FieldStorage" must be rebuilt, because it can contain
>   file objects. These file objects must be reset as they may
>   have been (partially) read in the previous request.
>   This prevent reusing the previous "FieldStorage".

As you may have seen in my first patch proposal I actually tried to
address this problem, although halfbaken and buggy.  So here's the
next trial.  I hope I've got it straight now.

Why didn't I do something to reset the files, FiledStorage creates?

Because I thought it wouldn't be needed.  If you look at
cgi.FieldStorage you will see, that accessing FieldStorage's 'value'
field, will always reset the file before reading it and HTTPRequest's
'BODY' field does the same.

Cause of your hint I looked again and realised that I was wrong.
HTTPRquest's 'BODYFILE' field fails to reset the file before accessing
it (as in OFS/Image.py the File class does).  That could be easily
fixed.  What's worse is that FiledStorage can possibly create
subinstances of itsellf which can possibly contain other fileobjects.
This is the case for multipart-type requests.

How to solve this?

I can see three alternative aproaches:

1) In HTTPRequest.retry loop through self._fs.list (if it exists) and
   call seek(0) on every fileobject, you can find there.  This can
   only work if the maximum depth of the FiledStorage tree is
   predictable, which in turn depends on the possible input.  I don't
   know enough about the possible HTTP requests to determine such a
   limit or to even 'prove' its existance.

2) In HTTPRequest.retry recursively walk through the FieldStorage
   trees in self._fs.list (if it exists).  I wouldn't do that in
   Python without being able to predict the (worst case) number of
   function calls needed to do this recursion.  And without refreshing
   my knowledge of compiler techniques I wouldn't be able to roll the
   recursion out to a loop.

3) This is a somewhat drastic (and possibly conceived as hackish?)
   approach which OTOH is independent of the input and should perform
   quite well.  I overwrite cgi.FieldStorage, esp. its make_file
   method, such that it adds all fileobjects, that it creates, to a
   (possibly caller provided) list.  Thus the caller
   (i.e. HTTPRequest) has a list of all those fileobjects that need to
   be reset if the FieldStorage should be reused (as in ZPublisher's

For the patch attached I chose the third approach.  Please let me
know, what you think about it.  If it is assessed to be ok I will
update the collctor item, I opened yesterday.



--- lib.orig/python/ZPublisher/HTTPRequest.py	2004-08-18 16:37:18.000000000 +0200
+++ lib/python/ZPublisher/HTTPRequest.py	2004-08-19 17:42:31.000000000 +0200
@@ -60,6 +60,37 @@
 class NestedLoopExit( Exception ):
+FILELIST_ENV_ENTRY = 'request.filestorage.filelist'
+class ZFieldStorage(cgi.FieldStorage):
+    """\
+    Tame the FieldStorage.
+    This class overrides the stock implementation to add all file
+    objects, generated while parsing a request, to a list.  This list
+    is given by the caller.
+    Unfortunately cgi.FieldStorage is not generic enough, to adapt the
+    list of constructor parameters, by just deriving from it.  Thus
+    the list is specified within the environ dict, given to the
+    constructor.  This is ugly and could be fixed by a simple patch
+    against cgi.FieldStorage.
+    """
+    def __init__(self, fp=None, headers=None, outerboundary="",
+                 environ=os.environ, keep_blank_values=0, strict_parsing=0):
+        self._filelist = environ.get(FILELIST_ENV_ENTRY, [])
+        cgi.FieldStorage.__init__(self, fp, headers, outerboundary,
+                                  environ, keep_blank_values,
+                                  strict_parsing)
+    def make_file(self, binary=None):
+        import tempfile
+        res = tempfile.TemporaryFile("w+b")
+        self._filelist.append(res)
+        return res
 class HTTPRequest(BaseRequest):
     Model HTTP request data.
@@ -125,10 +156,15 @@
     def retry(self):
-        self.stdin.seek(0)
+        if not self._fs:
+            raise ValueError, "Retrying on partially initialized request is not supported.  Call processInputs()."
+        for f in self._fs_filelist:
+            f.seek(0)
-                         response=self.response.retry()
+                         response=self.response.retry(),
+                         fs = self._fs,
+                         flist = self._fs_filelist
         return r
@@ -138,6 +174,8 @@
         # removing tempfiles.
         self.stdin = None
         self._file = None
+        self._fs = None
+        self._fs_filelist = None
         # we want to clear the lazy dict here because BaseRequests don't have
         # one.  Without this, there's the possibility of memory leaking
@@ -237,7 +275,7 @@
         return self._client_addr
-    def __init__(self, stdin, environ, response, clean=0):
+    def __init__(self, stdin, environ, response, clean=0, fs=None, flist=None):
         # Avoid the overhead of scrubbing the environment in the
         # case of request cloning for traversal purposes. If the
@@ -261,7 +299,11 @@
+        self._fs = fs
+        if flist is None:
+            self._fs_filelist = []
+        else:
+            self._fs_filelist = flist
         if environ.has_key('REMOTE_ADDR'):
             self._client_addr = environ['REMOTE_ADDR']
@@ -382,23 +424,26 @@
-        fs=FieldStorage(fp=fp,environ=environ,keep_blank_values=1)
-        if not hasattr(fs,'list') or fs.list is None:
+        if not self._fs:
+            fs_env = environ
+            fs_env[FILELIST_ENV_ENTRY] = self._fs_filelist
+            self._fs=ZFieldStorage(fp=fp,environ=fs_env,keep_blank_values=1)
+        if not hasattr(self._fs,'list') or self._fs.list is None:
             # Hm, maybe it's an XML-RPC
-            if (fs.headers.has_key('content-type') and
-                fs.headers['content-type'] == 'text/xml' and
+            if (self._fs.headers.has_key('content-type') and
+                self._fs.headers['content-type'] == 'text/xml' and
                 method == 'POST'):
                 # Ye haaa, XML-RPC!
                 global xmlrpc
                 if xmlrpc is None: import xmlrpc
-                meth, self.args = xmlrpc.parse_input(fs.value)
+                meth, self.args = xmlrpc.parse_input(self._fs.value)
                 self.maybe_webdav_client = 0
-                self._file=fs.file
+                self._file=self._fs.file
-            fslist=fs.list
+            fslist=self._fs.list
Zope-Dev maillist  -  [EMAIL PROTECTED]
**  No cross posts or HTML encoding!  **
(Related lists - 
 http://mail.zope.org/mailman/listinfo/zope )

Reply via email to