You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cocoon.apache.org by Geoff Howard <co...@leverageweb.com> on 2004/02/12 05:28:24 UTC
before we release - multipart uploads
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.
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.
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.
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.
Geoff
Re: before we release - multipart uploads
Posted by Sylvain Wallez <sy...@apache.org>.
Geoff Howard wrote:
> Sylvain Wallez wrote:
>
>> 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.
>>
>
> ...
>
>>> 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.
>
>
> Ok. Curious - is this the only case we have of a lifecycle interface
> applied to a non-component?
>
>>> 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).
>
>
> Interesting - I couldn't find where woody was using dispose(). Why
> would it need to dispose instead of setDisposeWithRequest()?
The caller of setDisposeWithRequest(false) becomes responsible for
disposing the Part, since the request won't do it. This is used by the
UploadWidget to handle form redisplay without loosing uploaded files:
when a file is uploaded, the corresponding part is detached from the
request and is disposed whenever the user replaces the already uploaded
file.
It's then the responsibility of the code using the form to dispose the
Part held by the widget when it has finished with it. Note that some
provision is made for applications that would forget this by deleting
temporary files in PartOnDisk.finalize(), and also by marking uploaded
files as deleteOnExit().
>>> 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.
>
>
> I think you're right that it's too late. The problem is that the
> misnamed accessor sounds like it performs an action and so may be used
> by people mistakenly. Would you be in favor of deprecating it right
> after release and creating a new method with the new name? Normally I
> am not so focused on naming issues, but this is a part of our API that
> will be touched by a lot of users who already seem to have a hard time
> with uploads and I'm anticipating a fresh round of questions...
Ok for deprecating, and I even think we could simply remame (and thus
loose the current name) it a this method is very internal to
MultiPartRequest and I doubt people will use it.
Sylvain
--
Sylvain Wallez Anyware Technologies
http://www.apache.org/~sylvain http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
Orixo, the opensource XML business alliance - http://www.orixo.com
Re: before we release - multipart uploads
Posted by Geoff Howard <co...@leverageweb.com>.
Sylvain Wallez wrote:
> 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.
...
>> 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.
Ok. Curious - is this the only case we have of a lifecycle interface
applied to a non-component?
>> 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).
Interesting - I couldn't find where woody was using dispose(). Why
would it need to dispose instead of setDisposeWithRequest()?
>> 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.
I think you're right that it's too late. The problem is that the
misnamed accessor sounds like it performs an action and so may be used
by people mistakenly. Would you be in favor of deprecating it right
after release and creating a new method with the new name? Normally I
am not so focused on naming issues, but this is a part of our API that
will be touched by a lot of users who already seem to have a hard time
with uploads and I'm anticipating a fresh round of questions...
Geoff
Re: before we release - multipart uploads
Posted by Sylvain Wallez <sy...@apache.org>.
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
--
Sylvain Wallez Anyware Technologies
http://www.apache.org/~sylvain http://www.anyware-tech.com
{ XML, Java, Cocoon, OpenSource }*{ Training, Consulting, Projects }
Orixo, the opensource XML business alliance - http://www.orixo.com