2011/5/20 Mark Thomas <ma...@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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
