Re: Fixes for bugs in WebdavResource putMethod and getMethod

Hello Michael,

I will definitively have a look at your bug reports once I get back  
to work on slide. I have been distracted from my Apache work since 2  
months.

Best regards,

Antoine
On Feb 8, 2007, at 5:46 PM, Michael Christoff wrote:

>
> Hi all.
>
> I just joined the list and didn't realize bug reports get sent here  
> directly.  So I figured I'd send my complete email directly to the  
> list as well as archiving it in bugzilla.
>
> This regards some new bug fixes I coded for the Slide client API.   
> The bug fixes apply to:
>
> org.apache.webdav.lib.WebdavResource.getMethod(String path, File  
> file), and
> org.apache.webdav.lib.WebdavResource.putMethod(String path, File file)
>
> I had previously submitted a patch for putMethod(String, File) and  
> had it accepted (see Slide 2.1 bug 40835, http://issues.apache.org/ 
> bugzilla/show_bug.cgi?id=40835 )--I was told it would appear in the  
> next release of the API. However I see there is still a problem  
> with this method even with the fix--and it turns out that the  
> problem also occurs in getMethod(String, File).
>
>
> /////////// **  PUTMETHOD BUG **
>
>
> Below is the orginal code for putMethod(String, File) :
>
>  ///////////////////////
>
>  1:    public boolean putMethod(String path, File file)
>  2:        throws HttpException, IOException {
>  3:
>  4:        setClient();
>  5:        PutMethod method = new PutMethod(URIUtil.encodePathQuery 
> (path));
>  6:        generateIfHeader(method);
>  7:        if (getGetContentType() != null && !getGetContentType 
> ().equals(""))
>  8:            method.setRequestHeader("Content-Type",  
> getGetContentType());
>  9:        long fileLength = file.length();
> 10:        method.setRequestContentLength(fileLength <=  
> Integer.MAX_VALUE
> 12:                                       ? (int) fileLength
> 13:                                       :  
> PutMethod.CONTENT_LENGTH_CHUNKED);
> 14:        method.setRequestBody(new FileInputStream(file));
> 15:        generateTransactionHeader(method);
> 16:        int statusCode = client.executeMethod(method);
> 17:
> 18:        setStatusCode(statusCode);
> 19:        return (statusCode >= 200 && statusCode < 300) ? true :  
> false;
> 20:    }
>
>  ///////////////////////
>
> As you can see, the FileInputStream created in line 14 is never  
> closed.  While this error won't manifest itself in a noticeable way  
> on *nix systems, on Windows a file opened for writing is locked and  
> hence applications on the client side will not be able to edit it  
> from therein (one would get a "File is in use" message).
>
> The original solution was to replace line 14 with following code:
>
> 14  :   FileInputStream fis = new FileInputStream(file);
> 14.1:   method.setRequestBody(fis);
>
> and then insert the following code after line 18:
>
> 18.1:   fis.close();
>
> However, the problem that still remains is this: what happens if an  
> exception is thrown between lines 15 and 18 (inclusive)?   
> Specifically, in line 16, the executeMethod of HttpClient may throw  
> an IOException or HttpException.  If this occurs then the  
> FileInputStream will still not be closed.  The solution to this is  
> below:
>
>
> /////////// **  PUTMETHOD FIX **
>
>
>  ///////////////////////
>
>  1:    public boolean putMethod(String path, File file)
>  2:        throws HttpException, IOException {
>  3:
>  4:        FileInputStream fis = null;
>  5:        try {
>  6:            setClient();
>  7:            PutMethod method = new PutMethod 
> (URIUtil.encodePathQuery(path));
>  8:            generateIfHeader(method);
>  9:            if (getGetContentType() != null && !getGetContentType 
> ().equals(""))
> 10:                method.setRequestHeader("Content-Type",  
> getGetContentType());
> 11:            long fileLength = file.length();
> 12:            method.setRequestContentLength(fileLength <=  
> Integer.MAX_VALUE
> 13:                                           ? (int) fileLength
> 14:                                           :  
> PutMethod.CONTENT_LENGTH_CHUNKED);
> 15:            fis = new FileInputStream(file);
> 16:            method.setRequestBody(fis);
> 17:            generateTransactionHeader(method);
> 18:            int statusCode = client.executeMethod(method);
> 19:
> 20:            setStatusCode(statusCode);
> 21:            return (statusCode >= 200 && statusCode < 300) ?  
> true : false;
> 22:
> 23:        } finally {
> 24:            if(fis != null)
> 25:                fis.close();
> 26:        }
> 27:    }
>
>  ///////////////////////
>
> By enclosing the method's code in a try block, we can ensure fis is  
> always closed, even if an exception occurs after it is instantiated.
>
>
> -------------------------
>
>
> /////////// **  GETMETHOD BUG **
>
>
> The identical problem exists in putMethod(String, File) of the same  
> class.  The original code is below:
>
>  ///////////////////////
>
>  1:    public boolean getMethod(String path, File file)
>  2:        throws HttpException, IOException {
>  3:
>  4:        setClient();
>  5:        GetMethod method = new GetMethod(URIUtil.encodePathQuery 
> (path));
>  6:
>  7:        generateTransactionHeader(method);
>  8:        int statusCode = client.executeMethod(method);
>  9:
> 10:        setStatusCode(statusCode);
> 11:
> 12:        // get the file only if status is any kind of OK
> 13:        if (statusCode >= 200 && statusCode < 300) {
> 14:
> 15:            // Do a simple little loop to read the response back  
> into the passed
> 16:            // file parameter.
> 17:            InputStream inStream = method.getResponseBodyAsStream 
> ();
> 18:
> 19:            FileOutputStream fos = new FileOutputStream(file);
> 20:            byte buffer[] = new byte[65535];
> 21:            int bytesRead;
> 22:            while ((bytesRead = inStream.read(buffer)) >= 0) {
> 23:                fos.write(buffer, 0, bytesRead);
> 24:            }
> 25:            inStream.close();
> 26:            fos.close();
> 27:
> 28:            return true;
> 29:
> 30:        } else {
> 31:            return false;
> 32:        }
> 33:   }
>
> If an exception is thrown between lines 20 and 24 (inclusive) the  
> input and output streams inStream and fos will not be closed.
>
>
>
> /////////// **  GETMETHOD FIX **
>
>
> The solution is the same as it was for putMethod(String, File) :
>
>  ///////////////////////
>
>  1:    public boolean getMethod(String path, File file)
>  2:        throws HttpException, IOException {
>  3:
>  4:        setClient();
>  5:        GetMethod method = new GetMethod(URIUtil.encodePathQuery 
> (path));
>  6:
>  7:        generateTransactionHeader(method);
>  8:        int statusCode = client.executeMethod(method);
>  9:
> 10:        setStatusCode(statusCode);
> 11:
> 12:        // get the file only if status is any kind of OK
> 13:        if (statusCode >= 200 && statusCode < 300) {
> 14:
> 14.1:          FileOutputStream fos = null;
> 14.2:          InputStream inStream = null;
> 14.3:          try {
> 15:                // Do a simple little loop to read the response  
> back into the passed
> 16:                // file parameter.
> 17*:               inStream = method.getResponseBodyAsStream();
> 18:
> 19*:               fos = new FileOutputStream(file);
> 20:                byte buffer[] = new byte[65535];
> 21:                int bytesRead;
> 22:                while ((bytesRead = inStream.read(buffer)) >= 0) {
> 23:                    fos.write(buffer, 0, bytesRead);
> 24:                }
> -25-
> -26-
> 27:
> 28:                return true;
> 28.1:
> 28.2:          } finally {
> 28.3:              if(fos != null)
> 28.4:                  fos.close();
> 28.5:              if(inStream != null)
> 28.6:                  inStream.close();
> 28.7:          }
> 29:
> 30:        } else {
> 31:            return false;
> 32:        }
> 33:   }
>
>  ///////////////////////
>
> Lines 14.1-14.3 and 28.1-28.7 are added code, lines 25 and 26 have  
> been deleted from the original source, and lines 17 and 19 have  
> been updated (we now declare inStream and fos outside the try block  
> in lines 14.1-14.3).
>
>
> Cheers!
>
>
>
> Michael N. Christoff
> Site Administrator
> Continuing Professional Education Online
> Centre for Addiction and Mental Health
> 33 Russell Street
> Toronto, Ontario M5S 2S1
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]