cocoon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sylvain Wallez <>
Subject Re: before we release - multipart uploads
Date Thu, 12 Feb 2004 09:18:53 GMT
Geoff Howard wrote:

> I dug into upload tonight to help some confusion on the users list and 
> noticed some new things which were introduced since 2.1.3 (in nov. by 
> Sylvain to support woody).  I'd like to propose some tweaks which seem 
> wise enough to me to get in quickly or it will be too late.
> 1) A new concept of disposeWithRequest (private boolean) was added to 
> org.apache.cocoon.servlet.multipart.Part to signal cleanup behavior at 
> the end of the request.  MultipartHttpServletRequest method cleanup is 
> called at the end of processing in CocoonServlet and is now:
>     /**
>      * Cleanup eventually uploaded parts that were saved on disk
>      */
>     public void cleanup() throws IOException {
>         Enumeration e = getParameterNames();
>         while (e.hasMoreElements()) {
>             Object o = get( (String)e.nextElement() );
>             if (o instanceof Part) {
>                 Part part = (Part)o;
>                 if (part.disposeWithRequest()) {
>                     part.dispose();
>                 }
>             }
>         }
>     }
> I like this (except the accessor name as noted below).
> 2) The method dispose() was added by having Part implement Disposable. 
> I like the method, but didn't like implementing a lifecycle method on 
> what is otherwise not a component.  I think dispose (or another name 
> if the overlapping name is deemed too confusing) should just be added 
> to the abstract Part without Disposable.

I personally don't see any problem with using this interface with 
non-components, rather the contrary. Disposable clearly identifies that 
some cleanup is required on the object, and everybody knows and 
understands its semantics: do some cleanup at the end of the object's 
life and never use it again. Sure, we can have dispose() without 
implementing the interface, but doing it so IMO gives better visibility 
to this important contract.

> 3) dispose() is public - but it could be protected, or even package 
> private.  I can't think of a reason why code outside this limited 
> scope (the cleanup method above) should be calling dispose() directly, 
> unless Woody has a use for it that I haven't found.

dispose() *needs* to be public, as Woody's upload widget calls it. This 
widget also calls setDisposeWithRequest(false).

> 3) A public accessor is created called boolean disposeWithRequest(). 
> This seems better called isDisposeWithRequest() and I don't see why it 
> needs to be public.  It's intended use is in the cleanup method above, 
> unless woody has a use for it that I haven't found.

I agree with your naming concern, and you're right in saying it doesn't 
need formally need to be public, as it's used only by MultipartRequest.

> Sorry I just noticed these things - I glanced at the cvs mail at the 
> time it went by but didn't notice the issues above until I dug through 
> tonight.

With the above comments, it seems to me the only problem is the 
disposeWithRequest() acessor. I guess it's too late now to change it 
(Carsten said "don't commit"), but this should not hurt much.


Sylvain Wallez                                  Anyware Technologies 
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
Orixo, the opensource XML business alliance  -

View raw message