You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Mark Thomas <ma...@apache.org> on 2012/10/09 23:27:56 UTC
Resources - merging back the changes
I believe from the various threads on the Resources implementation for
Tomcat 8 there is agreement that:
- DirContext is not the right basis for the Resources API
- Refactoring is required to simplify the multiple 'add-ons' and to
provide a sound basis for overlays (which may slip to Servlet 3.2)
- Any Resources implementation must provide in-memory caching
The sandbox Resources implementation addresses all of the above and
passes the Servlet and JSP TCKs as well as all the unit tests.
Given the above, is there any objection to merging the changes made in
the sandbox back to trunk?
Note that the following TODOs remain:
- Provide an option to exclude selected resources from the cache
- Remove the DirContext code
- Review the use of trailing '/'
- Determine if a custom URL scheme is required (and implement if it is)
- Review the interaction of WebappClassLoader and Resources to see if
there current work-arounds are a) still required b) implemented
efficiently
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Resources - merging back the changes
Posted by Mark Thomas <ma...@apache.org>.
On 01/11/2012 18:25, Mark Thomas wrote:
> On 01/11/2012 05:21, Costin Manolache wrote:
>> On Wed, Oct 31, 2012 at 4:41 PM, Costin Manolache <co...@gmail.com> wrote:
>>
>>> Hi Mark,
>>>
>>> I synced from HEAD - and noticed that DefaultServlet.PUT now fails with
>>> '403' if the resource already exists.
>>> I'm pretty sure this used to work, and at least for webdav it's supposed
>>> to replace the content.
>>> Is this a bug - or intended (the problem seems to be preResourceExists()
>>> check in StandardRoot.write()) ?
>>>
>>
>> Sorry - my mistake reading the code, with a debugger I see
>> preResourceExist() is fine, but DirResourceSet.write() has a check - and
>> from the javadoc the intent seems to be to not overwrite.
>>
>> IMHO it would be better to change the javadoc and implementation to allow
>> overwrites. Alternative is to
>> do a delete before in webdav/default servlet - but I don't see what use
>> case is optimized by not allowing overwrites.
>
> It looks like I didn't include an equivalent of rebind() in the new
> resources implementation. That was an oversight rather than an
> intentional decision. I'll fix that next.
Fixed in r1404724.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Resources - merging back the changes
Posted by Mark Thomas <ma...@apache.org>.
On 01/11/2012 05:21, Costin Manolache wrote:
> On Wed, Oct 31, 2012 at 4:41 PM, Costin Manolache <co...@gmail.com> wrote:
>
>> Hi Mark,
>>
>> I synced from HEAD - and noticed that DefaultServlet.PUT now fails with
>> '403' if the resource already exists.
>> I'm pretty sure this used to work, and at least for webdav it's supposed
>> to replace the content.
>> Is this a bug - or intended (the problem seems to be preResourceExists()
>> check in StandardRoot.write()) ?
>>
>
> Sorry - my mistake reading the code, with a debugger I see
> preResourceExist() is fine, but DirResourceSet.write() has a check - and
> from the javadoc the intent seems to be to not overwrite.
>
> IMHO it would be better to change the javadoc and implementation to allow
> overwrites. Alternative is to
> do a delete before in webdav/default servlet - but I don't see what use
> case is optimized by not allowing overwrites.
It looks like I didn't include an equivalent of rebind() in the new
resources implementation. That was an oversight rather than an
intentional decision. I'll fix that next.
Mark
> Costin
>
>
>
>>
>>
>> Costin
>>
>> On Tue, Oct 9, 2012 at 2:27 PM, Mark Thomas <ma...@apache.org> wrote:
>>
>>> I believe from the various threads on the Resources implementation for
>>> Tomcat 8 there is agreement that:
>>>
>>> - DirContext is not the right basis for the Resources API
>>> - Refactoring is required to simplify the multiple 'add-ons' and to
>>> provide a sound basis for overlays (which may slip to Servlet 3.2)
>>> - Any Resources implementation must provide in-memory caching
>>>
>>> The sandbox Resources implementation addresses all of the above and
>>> passes the Servlet and JSP TCKs as well as all the unit tests.
>>>
>>> Given the above, is there any objection to merging the changes made in
>>> the sandbox back to trunk?
>>>
>>> Note that the following TODOs remain:
>>> - Provide an option to exclude selected resources from the cache
>>> - Remove the DirContext code
>>> - Review the use of trailing '/'
>>> - Determine if a custom URL scheme is required (and implement if it is)
>>> - Review the interaction of WebappClassLoader and Resources to see if
>>> there current work-arounds are a) still required b) implemented
>>> efficiently
>>>
>>> Mark
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>>
>>>
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Resources - merging back the changes
Posted by Costin Manolache <co...@gmail.com>.
On Wed, Oct 31, 2012 at 4:41 PM, Costin Manolache <co...@gmail.com> wrote:
> Hi Mark,
>
> I synced from HEAD - and noticed that DefaultServlet.PUT now fails with
> '403' if the resource already exists.
> I'm pretty sure this used to work, and at least for webdav it's supposed
> to replace the content.
> Is this a bug - or intended (the problem seems to be preResourceExists()
> check in StandardRoot.write()) ?
>
Sorry - my mistake reading the code, with a debugger I see
preResourceExist() is fine, but DirResourceSet.write() has a check - and
from the javadoc the intent seems to be to not overwrite.
IMHO it would be better to change the javadoc and implementation to allow
overwrites. Alternative is to
do a delete before in webdav/default servlet - but I don't see what use
case is optimized by not allowing overwrites.
Costin
>
>
> Costin
>
> On Tue, Oct 9, 2012 at 2:27 PM, Mark Thomas <ma...@apache.org> wrote:
>
>> I believe from the various threads on the Resources implementation for
>> Tomcat 8 there is agreement that:
>>
>> - DirContext is not the right basis for the Resources API
>> - Refactoring is required to simplify the multiple 'add-ons' and to
>> provide a sound basis for overlays (which may slip to Servlet 3.2)
>> - Any Resources implementation must provide in-memory caching
>>
>> The sandbox Resources implementation addresses all of the above and
>> passes the Servlet and JSP TCKs as well as all the unit tests.
>>
>> Given the above, is there any objection to merging the changes made in
>> the sandbox back to trunk?
>>
>> Note that the following TODOs remain:
>> - Provide an option to exclude selected resources from the cache
>> - Remove the DirContext code
>> - Review the use of trailing '/'
>> - Determine if a custom URL scheme is required (and implement if it is)
>> - Review the interaction of WebappClassLoader and Resources to see if
>> there current work-arounds are a) still required b) implemented
>> efficiently
>>
>> Mark
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
>
Re: Resources - merging back the changes
Posted by Costin Manolache <co...@gmail.com>.
Hi Mark,
I synced from HEAD - and noticed that DefaultServlet.PUT now fails with
'403' if the resource already exists.
I'm pretty sure this used to work, and at least for webdav it's supposed to
replace the content.
Is this a bug - or intended (the problem seems to be preResourceExists()
check in StandardRoot.write()) ?
Costin
On Tue, Oct 9, 2012 at 2:27 PM, Mark Thomas <ma...@apache.org> wrote:
> I believe from the various threads on the Resources implementation for
> Tomcat 8 there is agreement that:
>
> - DirContext is not the right basis for the Resources API
> - Refactoring is required to simplify the multiple 'add-ons' and to
> provide a sound basis for overlays (which may slip to Servlet 3.2)
> - Any Resources implementation must provide in-memory caching
>
> The sandbox Resources implementation addresses all of the above and
> passes the Servlet and JSP TCKs as well as all the unit tests.
>
> Given the above, is there any objection to merging the changes made in
> the sandbox back to trunk?
>
> Note that the following TODOs remain:
> - Provide an option to exclude selected resources from the cache
> - Remove the DirContext code
> - Review the use of trailing '/'
> - Determine if a custom URL scheme is required (and implement if it is)
> - Review the interaction of WebappClassLoader and Resources to see if
> there current work-arounds are a) still required b) implemented
> efficiently
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
Re: Resources - merging back the changes
Posted by Mark Thomas <ma...@apache.org>.
On 23/10/2012 01:01, Konstantin Kolinko wrote:
> 2012/10/17 Mark Thomas <ma...@apache.org>:
>> On 17/10/2012 15:07, Konstantin Kolinko wrote:
>>
>>> 1. Are <PreResource> and <PostResource> able to inject individual files?
>>
>> Not yet, but that should be relatively simple to implement.
>>
>>> 2. I am -1 on removal of VirtualWebappLoader.
>>>
>>> The feature of injecting additional classpath entries into
>>> WebappClassLoader.super.addURL() is an important one and I use it a
>>> lot to inject external dependent libraries into web applications.
>>>
>>> I do not like your approach as a replacement, because classpath
>>> entries handling (jars handling) in URLClassLoader and in
>>> WebappClassLoader is based on different requirements. I think that
>>> URLClassLoader serves better for external libraries
>>
>> Could you expand on this please. I'm struggling to see the difference
>> between using the VirtualWebappLoader and mapping JARs into WEB-INF/lib
>> and directories containing classes onto WEB-INF/classes
>>
>
> There is a difference,
> but after thinking about it I am removing my veto.
OK. Thanks. I might start porting this back quite soon as I have some
test cases I want to write that would be a lot easier with the new
implementation.
> 1. If I consider the following two operations
> a. adding a library to the classpath
> b. maintaining it as a proper webapplication resource
>
> if "a." is all that I need, then having (a. + b.) will have an
> additional overhead.
Yes, there is an additional overhead. Exactly what that overhead is will
vary on a case by case basis and I'd hope was at least partially
compensated for by a simpler resources implementation.
> What VirtualWebappLoader does is essentially similar to putting a
> library into $CATALINA_BASE/shared.
>
>
> I know of the following overheads:
> 1.1.) An additional resource participates in resource lookups
>
> In my use case I insert a dozen of libraries into the classpath. I do
> not use VirtualWebappLoader directly, I am calling the API that it
> uses - WebappLoader.addRepository().
>
> Looking at the example of Tomcat 7 aliases, if you have 100 aliases
> then every resource lookup will perform a linear search through all
> the aliases.
>
> Regarding my question of injecting individual files into resources -
> maybe it would be better to support collections of individual files
> rather than files themselves. E.g. wrap a dozen of libraries into
> single collection for "/WEB-INF/lib".
You could place all the JARs you wanted to map in a single directory and
map that although if you want a single, central library of JARs from
which to work from you'd need a custom ResourceSet implementation which
should be pretty simple for the situation you describe.
> This way the individual libraries will be enumerated only when one is
> searching for something with a prefix of "/WEB-INF/lib". Searching for
> any other prefix will skip the libraries.
Agreed, with a custom ResourceSet.
Rather than a custom ResourceSet, the ResourceSet implementation could
be part of the standard distribution.
> 1.2.) WebappClassLoader#closeJARs(..)
> WebappClassLoader closes unused JARs after some time of inactivity.
>
> Discussed here:
> https://issues.apache.org/bugzilla/show_bug.cgi?id=52448
>
> I do not see much sense in this jar closing feature. Anyway,
> URLClassLoader keeps its jars open.
>
> (Not much of an issue, though. If it were a real nuisance, we can make
> it configurable).
It is probably worth some svn archaeology to see if we can figure out
why this feature was added in the first place.
> 2. There will be small difference with handling of
> VirtualWebappLoader#setSearchVirtualFirst( ) property.
>
> With VirtualWebappLoader and searchVirtualFirst=true the order of
> precedence when looking of a class is
> {external classes and libs, webapp classes, webapp libs}.
>
> With webresources I think the order will be different, because
> WEB-INF/classes takes precedence over WEB-INF/lib:
> {external classes, webapp classes, external libs, webapp libs}.
>
> It is a little nuance. I do not think there will be much confusion from it.
Agreed.
Cheers,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Resources - merging back the changes
Posted by Konstantin Kolinko <kn...@gmail.com>.
2012/10/17 Mark Thomas <ma...@apache.org>:
> On 17/10/2012 15:07, Konstantin Kolinko wrote:
>
>> 1. Are <PreResource> and <PostResource> able to inject individual files?
>
> Not yet, but that should be relatively simple to implement.
>
>> 2. I am -1 on removal of VirtualWebappLoader.
>>
>> The feature of injecting additional classpath entries into
>> WebappClassLoader.super.addURL() is an important one and I use it a
>> lot to inject external dependent libraries into web applications.
>>
>> I do not like your approach as a replacement, because classpath
>> entries handling (jars handling) in URLClassLoader and in
>> WebappClassLoader is based on different requirements. I think that
>> URLClassLoader serves better for external libraries
>
> Could you expand on this please. I'm struggling to see the difference
> between using the VirtualWebappLoader and mapping JARs into WEB-INF/lib
> and directories containing classes onto WEB-INF/classes
>
There is a difference,
but after thinking about it I am removing my veto.
1. If I consider the following two operations
a. adding a library to the classpath
b. maintaining it as a proper webapplication resource
if "a." is all that I need, then having (a. + b.) will have an
additional overhead.
What VirtualWebappLoader does is essentially similar to putting a
library into $CATALINA_BASE/shared.
I know of the following overheads:
1.1.) An additional resource participates in resource lookups
In my use case I insert a dozen of libraries into the classpath. I do
not use VirtualWebappLoader directly, I am calling the API that it
uses - WebappLoader.addRepository().
Looking at the example of Tomcat 7 aliases, if you have 100 aliases
then every resource lookup will perform a linear search through all
the aliases.
Regarding my question of injecting individual files into resources -
maybe it would be better to support collections of individual files
rather than files themselves. E.g. wrap a dozen of libraries into
single collection for "/WEB-INF/lib".
This way the individual libraries will be enumerated only when one is
searching for something with a prefix of "/WEB-INF/lib". Searching for
any other prefix will skip the libraries.
1.2.) WebappClassLoader#closeJARs(..)
WebappClassLoader closes unused JARs after some time of inactivity.
Discussed here:
https://issues.apache.org/bugzilla/show_bug.cgi?id=52448
I do not see much sense in this jar closing feature. Anyway,
URLClassLoader keeps its jars open.
(Not much of an issue, though. If it were a real nuisance, we can make
it configurable).
2. There will be small difference with handling of
VirtualWebappLoader#setSearchVirtualFirst( ) property.
With VirtualWebappLoader and searchVirtualFirst=true the order of
precedence when looking of a class is
{external classes and libs, webapp classes, webapp libs}.
With webresources I think the order will be different, because
WEB-INF/classes takes precedence over WEB-INF/lib:
{external classes, webapp classes, external libs, webapp libs}.
It is a little nuance. I do not think there will be much confusion from it.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Resources - merging back the changes
Posted by Mark Thomas <ma...@apache.org>.
On 17/10/2012 15:07, Konstantin Kolinko wrote:
> 1. Are <PreResource> and <PostResource> able to inject individual files?
Not yet, but that should be relatively simple to implement.
> 2. I am -1 on removal of VirtualWebappLoader.
>
> The feature of injecting additional classpath entries into
> WebappClassLoader.super.addURL() is an important one and I use it a
> lot to inject external dependent libraries into web applications.
>
> I do not like your approach as a replacement, because classpath
> entries handling (jars handling) in URLClassLoader and in
> WebappClassLoader is based on different requirements. I think that
> URLClassLoader serves better for external libraries
Could you expand on this please. I'm struggling to see the difference
between using the VirtualWebappLoader and mapping JARs into WEB-INF/lib
and directories containing classes onto WEB-INF/classes
> 3. Regarding the API.
> In the tests I see
> [[[
> tomcat.addContext("", System.getProperty("java.io.tmpdir"));
>
> File lib = new File("webapps/examples/WEB-INF/lib");
> - ctx.setAliases("/WEB-INF/lib=" + lib.getCanonicalPath());
> + ctx.setResources(new StandardRoot(ctx));
> + ctx.getResources().createWebResourceSet(
> + WebResourceRoot.ResourceSetType.POST, lib.getAbsolutePath(),
> + "/WEB-INF/lib", "");
> ]]]
>
> That "setResources" call seems to be excessive.
>
> Maybe getResources() should create a StandardRoot instance implicitly,
> or Tomcat.addContext() could create one, or StandardContext could
> create one.
Easy enough. I'll take a look.
> 4. org.apache.catalina.webresources.Cache
> + private ConcurrentMap<String,CachedResource> resourceCache =
> + new ConcurrentHashMap<>();
>
> It think it'd be better with "final" there.
Yep.
> If you resolve #2., then I do not mind the merge.
> I do not see much problems with the new APIs. I think we can go with them.
Great. I'll like to get better clarity on yoru thinking behind #2 before
I merge anything.
> For reference:
> The last merge from trunk to the branch was in r1397302, so I did a
> svn diff between trunk and trunk-resources branch state after that
> commit.
> The command is
> svn diff --old=https://svn.apache.org/repos/asf/tomcat/trunk@1397302
> --new=https://svn.apache.org/repos/asf/tomcat/sandbox/trunk-resources@1397302
>
> I uploaded the diff file here (its size is ~400KB):
> http://people.apache.org/~kkolinko/patches/2012-10-17_tc8_resources-vs-trunk_r1397302.diff
Nice. Thanks.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Resources - merging back the changes
Posted by Konstantin Kolinko <kn...@gmail.com>.
2012/10/16 Mark Thomas <ma...@apache.org>:
> On 09/10/2012 22:27, Mark Thomas wrote:
>> I believe from the various threads on the Resources implementation for
>> Tomcat 8 there is agreement that:
>>
>> - DirContext is not the right basis for the Resources API
>> - Refactoring is required to simplify the multiple 'add-ons' and to
>> provide a sound basis for overlays (which may slip to Servlet 3.2)
>> - Any Resources implementation must provide in-memory caching
>>
>> The sandbox Resources implementation addresses all of the above and
>> passes the Servlet and JSP TCKs as well as all the unit tests.
>>
>> Given the above, is there any objection to merging the changes made in
>> the sandbox back to trunk?
>
> It has been a week and no objections so I will go ahead and do this.
> However, I am currently working on the new HTTP Upgrade API, the new
> WebSocket API and converting trunk to use both. I plan to focus on that
> for the next few days and and merge the resources changes afterwards.
>
> Mark
>
>> Note that the following TODOs remain:
>> - Provide an option to exclude selected resources from the cache
>> - Remove the DirContext code
>> - Review the use of trailing '/'
>> - Determine if a custom URL scheme is required (and implement if it is)
>> - Review the interaction of WebappClassLoader and Resources to see if
>> there current work-arounds are a) still required b) implemented
>> efficiently
>>
1. Are <PreResource> and <PostResource> able to inject individual files?
2. I am -1 on removal of VirtualWebappLoader.
The feature of injecting additional classpath entries into
WebappClassLoader.super.addURL() is an important one and I use it a
lot to inject external dependent libraries into web applications.
I do not like your approach as a replacement, because classpath
entries handling (jars handling) in URLClassLoader and in
WebappClassLoader is based on different requirements. I think that
URLClassLoader serves better for external libraries
See also #1. above.
3. Regarding the API.
In the tests I see
[[[
tomcat.addContext("", System.getProperty("java.io.tmpdir"));
File lib = new File("webapps/examples/WEB-INF/lib");
- ctx.setAliases("/WEB-INF/lib=" + lib.getCanonicalPath());
+ ctx.setResources(new StandardRoot(ctx));
+ ctx.getResources().createWebResourceSet(
+ WebResourceRoot.ResourceSetType.POST, lib.getAbsolutePath(),
+ "/WEB-INF/lib", "");
]]]
That "setResources" call seems to be excessive.
Maybe getResources() should create a StandardRoot instance implicitly,
or Tomcat.addContext() could create one, or StandardContext could
create one.
4. org.apache.catalina.webresources.Cache
+ private ConcurrentMap<String,CachedResource> resourceCache =
+ new ConcurrentHashMap<>();
It think it'd be better with "final" there.
If you resolve #2., then I do not mind the merge.
I do not see much problems with the new APIs. I think we can go with them.
For reference:
The last merge from trunk to the branch was in r1397302, so I did a
svn diff between trunk and trunk-resources branch state after that
commit.
The command is
svn diff --old=https://svn.apache.org/repos/asf/tomcat/trunk@1397302
--new=https://svn.apache.org/repos/asf/tomcat/sandbox/trunk-resources@1397302
I uploaded the diff file here (its size is ~400KB):
http://people.apache.org/~kkolinko/patches/2012-10-17_tc8_resources-vs-trunk_r1397302.diff
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Resources - merging back the changes
Posted by Mark Thomas <ma...@apache.org>.
On 09/10/2012 22:27, Mark Thomas wrote:
> I believe from the various threads on the Resources implementation for
> Tomcat 8 there is agreement that:
>
> - DirContext is not the right basis for the Resources API
> - Refactoring is required to simplify the multiple 'add-ons' and to
> provide a sound basis for overlays (which may slip to Servlet 3.2)
> - Any Resources implementation must provide in-memory caching
>
> The sandbox Resources implementation addresses all of the above and
> passes the Servlet and JSP TCKs as well as all the unit tests.
>
> Given the above, is there any objection to merging the changes made in
> the sandbox back to trunk?
It has been a week and no objections so I will go ahead and do this.
However, I am currently working on the new HTTP Upgrade API, the new
WebSocket API and converting trunk to use both. I plan to focus on that
for the next few days and and merge the resources changes afterwards.
Mark
> Note that the following TODOs remain:
> - Provide an option to exclude selected resources from the cache
> - Remove the DirContext code
> - Review the use of trailing '/'
> - Determine if a custom URL scheme is required (and implement if it is)
> - Review the interaction of WebappClassLoader and Resources to see if
> there current work-arounds are a) still required b) implemented
> efficiently
>
> Mark
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org