Bug 52952

Summary:	Improve ExtensionValidator handling for embedded scenarios
Product:	Tomcat 7	Reporter:	Konstantin Kolinko <knst.kolinko>
Component:	Catalina	Assignee:	Tomcat Developers Mailing List <dev>
Status:	RESOLVED WONTFIX
Severity:	enhancement
Priority:	P2
Version:	7.0.26
Target Milestone:	---
Hardware:	PC
OS:	Windows XP
Attachments:	Patch Patch Patch

Description Konstantin Kolinko 2012-03-20 15:22:00 UTC

The following call was added in r1242101

In StandardServer#initInternal():

> +  ExtensionValidator.addSystemResource(f);

The above is a static method and it does not check for duplicates.

It does not scale well. E.g. if there are several Tomcat instances, e.g. run by JUnit.


I'd say that implementing ExtensionValidator with static methods is at fault here. The only place where it is called is

StandardContext#startInternal()
-> calls ExtensionValidator.validateApplication().

I do not see a need for static methods there. It should be possible to get a specific ExtensionValidator instance in StandardContext.

Comment 1 Mark Thomas 2012-03-20 20:17:06 UTC

Adding a duplicate check should be trivial.

There should be one validator per instance rather than one per context so adding it to the Server is likely the best solution.

Comment 2 Abdessamed MANSOURI 2016-04-26 00:26:11 UTC

For checking for duplicates its better to use sets than lists for storing Extension and ManifestResource inside ExtensionValidator and overriding equals and hashcode, but HashSet its based on HashMap and when HashMap is used the keys should be immutable, do i change Extension and ManifestResource to immutable?

Comment 3 Mark Thomas 2016-04-26 08:48:54 UTC

That should be fine.

Comment 4 Abdessamed MANSOURI 2016-04-27 13:35:42 UTC

Created attachment 33811 [details]
Patch

This is a patch for duplication with some code enhancements (replacing iterator loops with enhanced for-loop ...).

in this patch i didn't changed Extension to immutable class, changing JavaBean class to an immutable class made its instantiation (Long arguments in constructor or making new builder for the class), and i noticed that objects (Extension and ManifestResource) inside ExtensionValidator doesn't go out from it, so its nice to don't make them immutable cause they will not change outside :)

Comment 5 Abdessamed MANSOURI 2016-04-28 09:07:52 UTC

Created attachment 33813 [details]
Patch

Many unit test in the last patch didn't pass cause of many changes in the code, in this patch i have reverted everything and focusing only in this bug, now all unit tests pass :)

Comment 6 Mark Thomas 2016-05-18 19:40:59 UTC

Some feedback on the patch.

Objects is Java 7+. Tomcat 7 must run on Java 6 so that part of the patch needs re-work.

Otherwise the patch looks OK for what it does. What it misses is that there may be multiple Tomcat instances in an embedded scenario hence my comment about that rather than being static, ExtensionValidator needed to move to the Server so there could - in theory at least - be multiple independent instances in a JVM.

Comment 7 Abdessamed MANSOURI 2016-05-21 01:01:06 UTC

Created attachment 33858 [details]
Patch

I made ExtensionValidator non static and added it to the Server, then from Context i get it back by walking through the hiararchy.

I made also a global validator which belongs to the JVM, the idea is, if we couldn't get back the ExtensionValidator registred in the server we use this global validator (i don't want the server to suffer from NPE).

I have no idea how to write the unit case for ExtensionValidator (currently there's not test case) because available Extension and ManifestResource are different from JVM to other.

Comment 8 Mark Thomas 2018-06-08 14:09:07 UTC

The proposed patch looks to be heading along the right lines.

However, there hasn't been much interest in this enhancement either from users requiring or committers to perform a more detailed review and commit it.

In the meantime, Java has moved on. The extension mechanism has been removed from Java 9 onwards. It is very likely that the next Tomcat release will require at least Java 9 (more likely Java 11) and the ExtensionValidator code will be removed.

Given all of the above, I am resolving this as WONTFIX.