tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantin Kolinko <knst.koli...@gmail.com>
Subject Re: Fixing bug 33453
Date Mon, 23 May 2011 21:01:53 GMT
2011/5/20 Mark Thomas <markt@apache.org>:
> All,
>
> I've been looking at [1]. Ignoring the flames, there do appear to be
> several use cases where the current time-stamp checks are insufficient
> (although there are simple work-arounds). I have a patch [2] but I don't
> particularly like the fact that it breaks binary compatibility with JSPs
> compiled with an earlier version. My instinct is that this is bad. What
> does everyone else think?
>
> I do have an idea for addressing this:
> - Leave JspSourceDependent as is in 7.0.14 but deprecate it
> - Add a new JspSourceDependent2 interface (better names welcome)
> - Compilation always uses JspSourceDependent2
> - Isoutdated checked for JspSourceDependent as well as
> JspSourceDependent2 and any classes implementing JspSourceDependent are
> treated as outdated.
>
> Thoughts?
>
> Mark
>
> [1] https://issues.apache.org/bugzilla/show_bug.cgi?id=33453
> [2] https://issues.apache.org/bugzilla/attachment.cgi?id=27040&action=diff
>

Looking at the patch [2] what strikes me more is the new feature of
using setLastModified(jspLastModified); to set the timestamp of the
java and class files to match the original jsp(x)/tag(x) file.

It looks like a clever trick, but I am a bit afraid of it. It is a bit
counter-intuitive why timestamp can be earlier than max(timestamps of
dependencies).


If you use JspC to generate Java files for the pages and then have a
separate step to compile it you will notice, that "javac" task of Ant
[3] does not have "preservelastmodified" property, unlike the "copy"
task.

[3] http://ant.apache.org/manual/Tasks/javac.html


Not all war->file extract operations in Tomcat set lastmodifiedtime on
a file. This should be fixable though.  Searching for
"FileOutputStream"
+  ExpandWar.expand() does set the time.
-  WebappClassLoader.findResourceInternal(String, String) has "if
(antiJARLocking)" branch. It extracts resource files but does not set
lastmodifiedtime. I suspect that it may impact tag files in jars.
- WebappLoader.copyDir(DirContext, File) does not set the time.  IIRC
this happens when unpackWARs=false, though I do not see it straight
from the code.


Regarding the question in the original e-mail regarding the interface
my thoughts are:
Tim's suggestion looks like a better one. If we go with
"JspSourceDependent2", though:
a) I am OK with "JspSourceDependent2" name.
b) I think that old "JspSourceDependent" is of no concern for Tomcat,
but it might be for some tools that might run on top of it. So I think
it would be OK to write out Java files that implement both
"JspSourceDependent" and "JspSourceDependent2" interfaces.
Implementation of the old interface in this case is not needed to be
optimal and might be new ArrayList<String>(map.keySet()).
c) When loading the class do look for the "JspSourceDependent2"
interface only.  Absence of the new interface means the file is
outdated and needs to be regenerated and recompiled. It might be that
the "absence of interface" branch is already implemented.

Finally some small nitpicks regarding the patch [2] itself.
1) I wonder whether this is OK to change JspCompilationContext class
in this way. I think we usually add methods, but do not remove them.
2) In JspCompilationContext line 298 the change is
s/"jar:file:"/"jar:jndi:"/.  It might be needed, but I do not
understand why. It looks like a separate (un)related issue.
3) Typo in generated comment in Generator.java,  s/to assit/to assist/

Best regards,
Konstantin Kolinko

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message