Re: Fixing bug 33453

[Mark Thomas <ma...@apache.org>]

Getting back to this. Comments in-line.

On 23/05/2011 22:01, Konstantin Kolinko wrote:
> 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).

Hence the comment in the generated code. There was a comment on the bug
report that at least one other app server does this. Suggestions on what
else we can do welcome.

> 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.

I'm not concerned about JspC since there is never a dependency check for
recompilation in that case.

> 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.

Agreed. I'll include a fix for this in the patch.

> - 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.

It looks to me like that only extracts files from the classes directory.
I didn't look into why it does that, but it won't affect JSP compilation.

> Regarding the question in the original e-mail regarding the interface
> my thoughts are:
> Tim's suggestion looks like a better one.

I went with a variation of this. The change to using != rather than <
means that most JSPs / tag files will get recompiled on first access
anyway. I forced my way past that with the debugger and saw that an
AbstractMethodError is thrown. I can catch that and return something
that forces a recompilation.

> 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.

The release notes make clear that the internal API is subject to change
between point releases.

That said, keeping the original method and marking it as deprecated
won't do any harm so I'll do that.

We can't handle down-grades as easily, so I'll make sure that change log
includes a note on this.

> 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.

Sort of. Fixed separately.

> 3) Typo in generated comment in Generator.java,  s/to assit/to assist/

Thanks. Fixed.

Commit coming soon once I have finished some more testing.

Mark



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