I've noticed that Tomcat won't recompile a JSP file if the date stamp is changed to go back in time. This may seem like a strange case, but if you check an older copy of a JSP page out of version control, it's different, and needs to be recompiled. The assumption that all changes to a file involve a newer file datestamp is an invalid one. I think I found the code that makes this decision, in org.apache.jasper.compiler.Compiler, in the isOutDated(boolean) method. The condition is if (targetLastModified < jspRealLastModified) but it should be if (targetLastModified != jspRealLastModified) in my opinion. After all, the logic should be that the file has changed, not that it's newer. I don't think it's reasonable to expect that Jasper check the size and do an MD5 checksum to *really* see if the file has changed. :) Obviously the workaround is to just to "touch" the file but this adds a lot of overhead (and one more thing to remember), compared to changing a > to a !=.
Changes to a file that make the change timestamp older instead of newer seems like a special (if not outright wrong) use-case. I'm not sure Tomcat should worry about this at all.
(In reply to comment #1) > Changes to a file that make the change timestamp older instead of newer seems > like a special (if not outright wrong) use-case. I don't agree that using version control is a special or wrong use case. In fact, I think the opposite is true. Example: 1) Check out a JSP file from VC and deploy it where Tomcat can see it 2) Access the JSP file via a web browser, so it gets compiled 3) Change it locally, deploy the changed version (newer timestamp) 4) Access it via a web browser; Tomcat correctly shows the updated version. 5) Revert to an older version from VC and deploy that. 6) Access it via a web browser; Tomcat ignores the changes and shows the output of a nonexistent JSP page
This is an enhancement at best. (That I would be -0 to)
Although not allowed to vote I have an opinion: The dynamic update feature for JSPs comes from the need to dynamically manage content without a heavy weight administrative action. But every now and then one has to roll back a change. With static content you would just put back the original files. It would be very nice, if it would work the same way with JSPs, especially because that's unsually the unplanned case (roll back) where you need to have a simple procedure. I know, you could touch the old file. Myself I always tell Sysadmins to make backup copies and roll back file changes with "cp -p", so that the files keep their original timestamp, which is a nice low-level approximation of checksums. Any higher level tool (like CVS) will most liekely also roll back including file timestamps. There seems to be very little risk in the change and some not neglectable benefit.
Please test this *really* well. Also, you should test with a similar change made to the dependent files near the end of the isOutDated method (otherwise, touch would still be required in some cases).
Thanks for your hint. I do like the use case, but I should have thought better about the implementation suggestion of Jamie. His idea to just compare for timestamps for not being equal will not work! In most cases the timestamps of the class files will of course be younger than the source file (source file has some old installation date but class file is only generated on first access). So this change would make the compiler compile on every test :( It would only work by saving the last JSP timestamp for any JSP and then comparing to the saved value insted of comparing to the timestamp of the generated files. I leave it up to Jamie to suggest a working patch - I don't knw enough about Jasper details.
Fair enough; that's not the place to make that change. The servlet .class or .java file will be slightly newer than the JSP file. It's the JSP file's date that should be compared to the cached last-modified time, regardless of how many ms it took to get around to generating and compiling the actual servlet.
Feel free to reopen this when you have a patch ready for us to evaluate -- looking forward to it.
The .jsp file date stamp doesn't have to go back in time for the isOutDated check to fail, it can and does fail in a more normal usage pattern. Here is a scenario that shows the problem: - I deploy version 1, the .jsp has time1 - I make version 2 of the .jsp at time2 - Visitor visits the site, and the .jsp is compiled at time3 - I deploy version2 - isOutDated returns false as time3 > time2 Would setting the date stamp of the .java and .class files to the date stamp of the .jsp file, and changing the comparison from < to != in the isOutDated check fix the problem sufficiently? Or are there negative side effects I haven't thought of? I am working on patching my Tomcat to do exactly as above, I would be happy to give it to someone for evaluation when its ready.
(In reply to comment #9) > The .jsp file date stamp doesn't have to go back in time for the isOutDated > check to fail, it can and does fail in a more normal usage pattern. > Here is a scenario that shows the problem: > - I deploy version 1, the .jsp has time1 > - I make version 2 of the .jsp at time2 > - Visitor visits the site, and the .jsp is compiled at time3 > - I deploy version2 > - isOutDated returns false as time3 > time2 > > Would setting the date stamp of the .java and .class files to the date stamp of > the .jsp file, and changing the comparison from < to != in the isOutDated check > fix the problem sufficiently? Or are there negative side effects I haven't > thought of? > > I am working on patching my Tomcat to do exactly as above, I would be happy to > give it to someone for evaluation when its ready. As looked into in comment #6, this is not doable easily, which makes the few use cases which could benefit from this not worth it. Please try to read the report next time.
Remy, With all due respect I did read the report fully and I believe the recommendation I made addressed comment #6. If you feel my recommendation isn't sufficient, please state why. The other point in my first comment was that this bug can manifest itself in ways more common than the original report, in fact that's why my colleague and I found it. I have patched and tested my local Tomcat, and am attaching the two files I modified for review. (In reply to comment #10) > (In reply to comment #9) > > The .jsp file date stamp doesn't have to go back in time for the isOutDated > > check to fail, it can and does fail in a more normal usage pattern. > > Here is a scenario that shows the problem: > > - I deploy version 1, the .jsp has time1 > > - I make version 2 of the .jsp at time2 > > - Visitor visits the site, and the .jsp is compiled at time3 > > - I deploy version2 > > - isOutDated returns false as time3 > time2 > > > > Would setting the date stamp of the .java and .class files to the date stamp of > > the .jsp file, and changing the comparison from < to != in the isOutDated check > > fix the problem sufficiently? Or are there negative side effects I haven't > > thought of? > > > > I am working on patching my Tomcat to do exactly as above, I would be happy to > > give it to someone for evaluation when its ready. > > As looked into in comment #6, this is not doable easily, which makes the few use > cases which could benefit from this not worth it. Please try to read the report > next time. >
Created attachment 16489 [details] Changes to Compiler.java and JspCompilationContext.java to fix bug 33453 Modified Compiler.java isOutDated() method to use != instead of > for datestamp comparison. Modified JspCompilationContext.java compile() method to set the datestamp of the generated .java and .class files to the datestamp of the source .jsp.
Remy, I will keep re-opening this bug until you take the time to explain to me why I am wrong. Which even you can't, because I'm not.
Sorry for letting you down, I was having dinner.
Since when is it a good idea to *close* a bug that you can't think of how to fix offhand, and don't feel like fixing yourself? This behavior is infantile and is an embarassing contradiction to the spirit of the open source development model. And now you're refusing to look at a patch just to save face? This is really sad. I wonder how many other Tomcat bugs exist but were closed because somebody didn't want to think about them or try to resolve them. Maybe you ought to use the priority and target milestone features instead of pretending that Tomcat does the right thing because whatever it currently does defines what "the right thing" is. I'm not going to reopen this because I'm not wasting any more time on this little power game. There are competitors to Tomcat and this is just another reason to use them instead.
Created attachment 16492 [details] Changes to JspCompilationContext.java Sets the lastModified on the generated .java and .class files to the lastModified of the source .jsp
Created attachment 16493 [details] Change to Compiler.java Change the < to != in the isOutDated method.
As suggested by Rainer, I have submitted my recommended changes in patch form for easier review. Remy, I was also at lunch so unfortunately I was delayed in re-opening the bug, which I likewise apologize for. I accept your apology. I am considering creating a script to automate my re-opening of the bug. As a peace offering I am willing to automate your part, automatically changing the status to RESOLVED WONTFIX, just send me your bugzilla login and password. By the way, if you bothered to take the time to think about the problem, you would realize as I have that the current behavior is very broken. As I have stated before, not just for the original use case in the bug report, it affects every .jsp modification, without moving the timestamp backward whatsoever. This bug affects everyone, period.
My instinct is that changing the true timestamp of generated files is going to cause other problems. I am very uneasy with a solution that means what a user sees (in terms of timestamps) isn't going to be what actually happened. It is also worth having a look at bug 23406. In that case timing resolutions to the nearest second were not sufficient to resolve the issue. Any patch for this issue should also address the issue in 23406. My preference would be for a patch that recorded, for each JSP, somthing that is guaranteed to change for all of the related use cases. Timestamp + file size should be OK, MD5 certainly would be. My general unease about changing file timestamps hasn't got to the point where I would -1 this patch but I haven't had time to reflect on this yet. In summary: - I agree this is a problem - I agree the right solution isn't going to be easy - I think we need a more robust solution that the current patch.
(In reply to comment #19) > In summary: > - I agree this is a problem I disagree. > - I agree the right solution isn't going to be easy I do not wish to find any solution to this non issue. The only problem is that people will have to use touch or similar in a few very select situations, which apparently is too difficult. > - I think we need a more robust solution that the current patch. Yes, -1 for it. Obviously, anyone is free to waste his time on this trying to find an acceptable solution, but the said solution has better be trivial, otherwise it will get a -1 from me.
Setting timestamp seems not a clean solution to me. API doc says, hat the time might be rounded, so even if we try to set the time to the same timestamp we get from the JSP, the resulting timestamp might differ and not be equal to the original time (Consider diffrent file system types etc.). I would also prefer a solution where information about the JSP is saved and later compared. Would JspServletWrapper be the right place to save the original JSP modification time? MD5 would be nice, but then md5 checksum would need to be recalculated on every JSP check with unchanged file time, so unfortunately not a rare case. I guess that's too bad for performance. Maybe timestamp and size would be enough, because both can be retrieved easy and efficiently, and if timestamp did not change, but content did change, it is very likely, that the file was in progress of being written to, so at least size should have changed. If we agree, that it's worth trying to make a patch to JspServletWrapper, I'll try to submit one tomorrow (not really for 5.5.12). One thing remains though: I'm not sure how to handle the case of included JSPs (dependecies). Maybe I'll find a solution by digging deeper into Jasper. One last word: I had customers having problems with both scenarios: rolling back file changes, but also distributing content with wrong timestamps (future time) and in consequence continuous recompilation for several minutes. Not trying to assume a simple time model seems to make jasper more robust.
(In reply to comment #21) > I would also prefer a solution where information about the JSP is saved and > later compared. Would JspServletWrapper be the right place to save the original > JSP modification time? Nope, people can restart the container. > MD5 would be nice, but then md5 checksum would need to be recalculated on every > JSP check with unchanged file time, so unfortunately not a rare case. I guess > that's too bad for performance. Arg MD5. > Maybe timestamp and size would be enough, because both can be retrieved easy and > efficiently, and if timestamp did not change, but content did change, it is very > likely, that the file was in progress of being written to, so at least size > should have changed. This is simple, and maybe acceptable, but would make the cost of checking for recompilation (even) more expensive than it is right now. > One last word: I had customers having problems with both scenarios: rolling back > file changes, but also distributing content with wrong timestamps (future time) > and in consequence continuous recompilation for several minutes. Not trying to > assume a simple time model seems to make jasper more robust. On access compilation and its friend the development mode - which you are using or you would not have this "issue" - should not be used in production (the only reason why it is not as bad as it used to be is that I tweaked it do do only one check at most per page per time interval - obviously if there are 100 pages, my trick will not work that well).
You disagree because you don't fully understand the problem. If someone first visits your .jsp after your modification, but before redeployment, you will be hit by this bug. You will wonder why your change didn't take effect. It is not necessary for the timestamp to move backwards. You don't control when your .jsp is accessed. As your development/test servers will see different access than your production, you will encounter a production bug that you didn't see in your other environments. Or if you are load balanced you will encounter the bug on one machine but not another. Good luck debugging that when it happens to you. Using touch is not difficult, just add it to the documentation for JBoss and every other web server that to hot deploy, do the following: hot deploy, find the tmp directory where the war is unpacked, touch every .jsp file that changed. And do that atomically so that noone can visit the .jsp inbetween. The jasper code as is fails badly, my fix is trivial, is an improvement, but still not 100% correct and has the side effect of changing a couple timestamps. The only place I'm aware of in Jasper that uses those timestamps is the broken isOutDated logic. My fix doesn't handle dependencies correctly, and there is still the possibility the .jsp is visited in the same second / minute / whatever the OS granularity is as the modification. Another side effect of my fix is that every .jsp will be recompiled once after the fix is applied. Timestamp rounding by the filesystem is not an issue, the timestamp of the .jsp will be rounded the same way as the .class and .java files. Every OS will round timestamps in an internally consistent manner. If changing the timestamps of the .java and .class files is still deemed verboten, then I suggest copying the .jsp to the same temporary directory that the .java and .class files are generated in, and preserving its timestamp. Then timestamp, filesize, md5, even the exact contents of the file can be compared. Remy, I am using JBoss4.0.2, how should I be deploying .jsps to production to avoid this issue?
For what it's worth: A few years ago we implemented the timestamp approach to this issue in the WebSphere Application Server JSP container at the request of a small number of customers - for whom it was critical. A generated classfiles is set to the timestamp of the source JSP file. The classfile is considered to be outdated when the two timestamps do not match. File size is not part of the equation. Tag files are handled the same way. The timestamp disconnect :) of classfiles vs. their actual compilation time rarely causes confusion among customers; it's a non-issue. We write compilation time/date and other information into the generated .java file, in a comment, so any confusion that might occur can be easily cleared up. The timestamp != strategy has worked well on Versions 4 through 6, on all platforms. Dependency tracking (static includes, TLDs, tag files) is easily managed. The race condition described in bug 23406 has never been reported. Timestamp rounding has never been an issue. Google for "websphere jsp timestamp" and you'll find some info about the implementation. Some things to consider if you all decide it's an appropriate change for Tomcat (this stuff is all documented and easily found on the web): When serving JSP sources from JARs, we use the timestamp of the JAR for the outdated check. Any expansion of WARs or other compressed artifacts with precompiled JSP classes must maintain timestamps (doh). There *will* be first-time recompilation cost to Tomcat users if this is implemented, as Jonathan mentioned. Some won't like it. Keeping data only in a runtime artifact like the servletwrapper won't work, as Remy stated.
Another thing that currently compunds this issue is the fact that the zip file format used for .war files ignores timezone on datestamps. So for example, my file times are mountain standard and my server is in GMT. If I make a change to the .jsp after it is accessed on the server, unless it is 7 or 8 hours after, it won't take effect. And if the time change is in the reverse direction, the .jsp will be recompiled continuously for the duration of the difference.
I took my Tomcat out of development mode, and verified this issue exists there as well. development=false uses the same, broken isOutDated check. > On access compilation and its friend the development mode - which you are using > or you would not have this "issue" - should not be used in production
> On access compilation and its friend the development mode - which you are using > or you would not have this "issue" - should not be used in production By "access compilation" are you referring to the development="true" mode which causes recompilation on every access? We rely on our JSP pages to be compiled when the date changes and the only *reliable* way to do this is to touch *all* JSP files whenever we change something. I suspect it is because of the problem Jonathon has pointed out. Unfortunately, this means the webapp is very slow after each deployment. If this problem won't be fixed, what is the recommended way to avoid recompiling all pages in a production environment?
Created attachment 16599 [details] rebuilt jasper-compiler.jar Here is a rebuilt jasper-compiler.jar that incorporates my proposed patch. For anyone who needs a fix and doesn't want to download, patch, and rebuild.
We got bit by this bug again today! How can I impress on the developers the seriousness of this issue? We need to touch every JSP file when we deploy a webapp because we cannot trust that Tomcat will recompile the things that need to be. This causes large delays to the end users that are unlucky enough to hit the website first. If you revert your JSP files to an older branch, you also have to remember to touch them (many CM systems revert the dates to the older version which still pass the < comparison). I think Jonathon's fix will address every realistic scenario. Please use his patch!
(In reply to comment #28) > Created an attachment (id=16599) [edit] > rebuilt jasper-compiler.jar > > Here is a rebuilt jasper-compiler.jar that incorporates my proposed patch. For > anyone who needs a fix and doesn't want to download, patch, and rebuild. Hello Jonathan, i've tried tu use your jasper-compiler.jar into TC 5.5.12, because we have a problem to make Tomcat reload and compile modified JSPs on fly, but an exception is thrown when Jasper try to compile the JSP. i haven't try to patch an rebuilt yet. Here is the root cause of the stack trace, it seem like an import from an Eclipse library : java.lang.NoSuchMethodError: org.eclipse.jdt.internal.compiler.env.NameEnvironmentAnswer.<init>(Lorg/eclipse/jdt/internal/compiler/env/IBinaryType;)V org.apache.jasper.compiler.JDTCompiler$1.findType(JDTCompiler.java:214) org.apache.jasper.compiler.JDTCompiler$1.findType(JDTCompiler.java:183) org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment.askForType(LookupEnvironment.java:119) org.eclipse.jdt.internal.compiler.lookup.PackageBinding.getTypeOrPackage(PackageBinding.java:178) org.eclipse.jdt.internal.compiler.lookup.Scope.getPackage(Scope.java:2111) org.eclipse.jdt.internal.compiler.ast.QualifiedTypeReference.getTypeBinding(QualifiedTypeReference.java:62) org.eclipse.jdt.internal.compiler.ast.TypeReference.resolveType(TypeReference.java:141) org.eclipse.jdt.internal.compiler.ast.TypeReference.resolveSuperType(TypeReference.java:104) org.eclipse.jdt.internal.compiler.lookup.ClassScope.findSupertype(ClassScope.java:1088) org.eclipse.jdt.internal.compiler.lookup.ClassScope.connectSuperclass(ClassScope.java:755) org.eclipse.jdt.internal.compiler.lookup.ClassScope.connectTypeHierarchy(ClassScope.java:927) org.eclipse.jdt.internal.compiler.lookup.CompilationUnitScope.connectTypeHierarchy(CompilationUnitScope.java:254) org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment.completeTypeBindings(LookupEnvironment.java:195) org.eclipse.jdt.internal.compiler.Compiler.beginToCompile(Compiler.java:301) org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:315) org.apache.jasper.compiler.JDTCompiler.generateClass(JDTCompiler.java:387) org.apache.jasper.compiler.Compiler.compile(Compiler.java:288) org.apache.jasper.compiler.Compiler.compile(Compiler.java:267) org.apache.jasper.compiler.Compiler.compile(Compiler.java:255) org.apache.jasper.JspCompilationContext.compile(JspCompilationContext.java:557) org.apache.jasper.servlet.JspServletWrapper.service(JspServletWrapper.java:293) org.apache.jasper.servlet.JspServlet.serviceJspFile(JspServlet.java:291) org.apache.jasper.servlet.JspServlet.service(JspServlet.java:241) javax.servlet.http.HttpServlet.service(HttpServlet.java:802)
Fabien, The jasper-compiler.jar I built was against 5.5.9, I think that's the problem. It should work just fine if you patch and rebuild. -Jonathan (In reply to comment #30) > (In reply to comment #28) > > Created an attachment (id=16599) [edit] [edit] > > rebuilt jasper-compiler.jar > > > > Here is a rebuilt jasper-compiler.jar that incorporates my proposed patch. For > > anyone who needs a fix and doesn't want to download, patch, and rebuild. > > Hello Jonathan, > > i've tried tu use your jasper-compiler.jar into TC 5.5.12, because we have a > problem to make Tomcat reload and compile modified JSPs on fly, but an exception > is thrown when Jasper try to compile the JSP. > > i haven't try to patch an rebuilt yet. > > Here is the root cause of the stack trace, it seem like an import from an > Eclipse library : > > java.lang.NoSuchMethodError: > org.eclipse.jdt.internal.compiler.env.NameEnvironmentAnswer.<init>(Lorg/eclipse/jdt/internal/compiler/env/IBinaryType;)V > org.apache.jasper.compiler.JDTCompiler$1.findType(JDTCompiler.java:214) > org.apache.jasper.compiler.JDTCompiler$1.findType(JDTCompiler.java:183) > org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment.askForType(LookupEnvironment.java:119) > org.eclipse.jdt.internal.compiler.lookup.PackageBinding.getTypeOrPackage(PackageBinding.java:178) > org.eclipse.jdt.internal.compiler.lookup.Scope.getPackage(Scope.java:2111) > org.eclipse.jdt.internal.compiler.ast.QualifiedTypeReference.getTypeBinding(QualifiedTypeReference.java:62) > org.eclipse.jdt.internal.compiler.ast.TypeReference.resolveType(TypeReference.java:141) > org.eclipse.jdt.internal.compiler.ast.TypeReference.resolveSuperType(TypeReference.java:104) > org.eclipse.jdt.internal.compiler.lookup.ClassScope.findSupertype(ClassScope.java:1088) > org.eclipse.jdt.internal.compiler.lookup.ClassScope.connectSuperclass(ClassScope.java:755) > org.eclipse.jdt.internal.compiler.lookup.ClassScope.connectTypeHierarchy(ClassScope.java:927) > org.eclipse.jdt.internal.compiler.lookup.CompilationUnitScope.connectTypeHierarchy(CompilationUnitScope.java:254) > org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment.completeTypeBindings(LookupEnvironment.java:195) > org.eclipse.jdt.internal.compiler.Compiler.beginToCompile(Compiler.java:301) > org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:315) > org.apache.jasper.compiler.JDTCompiler.generateClass(JDTCompiler.java:387) > org.apache.jasper.compiler.Compiler.compile(Compiler.java:288) > org.apache.jasper.compiler.Compiler.compile(Compiler.java:267) > org.apache.jasper.compiler.Compiler.compile(Compiler.java:255) > org.apache.jasper.JspCompilationContext.compile(JspCompilationContext.java:557) > org.apache.jasper.servlet.JspServletWrapper.service(JspServletWrapper.java:293) > org.apache.jasper.servlet.JspServlet.serviceJspFile(JspServlet.java:291) > org.apache.jasper.servlet.JspServlet.service(JspServlet.java:241) > javax.servlet.http.HttpServlet.service(HttpServlet.java:802)
I think the patch files are backwards (show how to remove the fix) but that doesn't make this bug less important. We just got bit by this thing again... every couple of month at my company. It's time to bite the bullet and make a one-off version. Is it ever going to be fixed?
(In reply to comment #32) Sorry for getting the patch backwards. Not that it matters, it will probably never be incorporated any way, due to the co-location of a certain person's head with his ass. Do the votes matter? I am the only one voting for this, perhaps if a few more people used their votes it would show up on a list somewhere, causing some action to be taken. Other than patching Tomcat, here's what I recommend, in order of preference: 1) Don't use Tomcat. 2) Don't use JSPs. 4) Precompile your JSPs as part of your build process.
Jonathoan, I was about to try using your patch and noticed a problem. You are using the date of the JSP file to stamp the class file but the isOutDated() method is also taking into account the times of the included JSP files. This can cause a JSP file to be re-compiled on every hit if it includes a file that has a later date. For example, if a.jsp is time-stamped 9:00 and it includes b.jsp which stamped 10:00, then isOutDated () will always return true because the time of the included file is greater than 9:00. Then when you assign the earlier time to the class file, this check will fail again the next time around. I think a better solution is to set the time of the class file to the greatest time of the compiled file AND ALL of it's dependants. In the above scenario, the class file would get a time stamp of 10:00 instead of 9:00 and subsequent checks of isOutDated would return false as expected. The only problem I can forsee is if someone updates the included file, THEN rolls back the original file. That seems like a very obscure case and my solution is still better than the current solution which doesn't account for rollbacks at all (and also wouldn't handle the obscure case). I will attempt a solution but will be on vacation soon so it may be a while before I'm able to post any patches.
(In reply to comment #34) Tom, I see the problem with the dependants; that is a huge problem with my patch. I think it is worth solving this problem completely, in a manner similar to the way Websphere does (see comment #24 and do the google search). To do this, it is necessary to compare the current and last timestamps of the jsp and all its dependants. Websphere persists this information in the comments of the generated .java file. I don't like the idea of putting the timestamps in the comments ala Websphere, it will mean making changes in more places, and also I am not a fan of parsing text. I am thinking along the lines of creating a seperate file which could be as simple as a serialized hashmap containing the URLs as keys and the timestamps as values. Thoughts?
Jonathan, just to clairfy: WebSphere doesn't store the timestamp information in comments - what is stored in comments is informational data that can be used to help debug problems. The timestamps used by the outdated checks are stored in the generated classfile as part of the _jspx_dependants List. If you were to look at the generated .java source you would see, for example: private static String[] _jspx_dependants; static { _jspx_dependants = new String[2]; _jspx_dependants[0] = "/Banner.jsp^1082410708000^Mon Apr 19 17:38:28 EDT 2004"; _jspx_dependants[1] = "/Footer.jsp^1077657462000^Tue Feb 24 16:17:42 EST 2004"; } The timestamp simply follows a dependent's path information. The data in comments the I referred to in post #24 is more this sort of thing: e:/mytempdir/x.ear/y.war/WEB-INF/classes/_ibmjsp/_jsp1.java was generated @ Thu Mar 16 14:03:16 EST 2006 IBM WebSphere Application Server - ND, 6.1.0.0 Build Number: v0611.54 Build Date: 3/16/06 ******************************************************** The JSP engine configuration parameters were set as follows: classDebugInfo = [false] debugEnabled = [false] deprecation = [false] compileWithAssert = [false] etc. etc. etc. Hope this helps.
(In reply to comment #36) Scott, thanks! That helps a whole lot. I hadn't considered storing the lastModified times of the dependencies as member data of the servlet. It is the ideal place. In fact, Tomcat already stores the dependency list there. I am currently testing a fix which stores the dependant lastModified times there, as well as the lastModified time of the .jsp (rather than modify the timestamp of the .class and .java files). The comparisons made are all != instead of > . I will post a new patch once I've tested my changes more thorougly.
Jonathan, I have one caveat about using the servlet member data for the storing the timestamp of the JSP itself and using it for outdated check instead of using the JSP source and class file timestamps. The problem is that the servlet class has to be loaded in order to retrieve this data. Therefore, you wouldn't know a servlet class file was outdated until you'd already loaded it once (first request). We decided that this 'lag' was ok for dependency checking (which by the way is turned off by default in WebSphere) but did not like this lag for the top-level JSP reloading. As I said, just a caveat.
(In reply to comment #38) Scott, I think I understand your point, but not fully. Is the lag you refer to the lag of loading the class twice vs. once, in the case of the .jsp being outdated and its class not already loaded? As I understand it, if either the .jsp is not outdated, or the class is already loaded, there would be no extra lag. Is that correct? I think I might be missing something, or perhaps Tomcat works differently from Websphere. Or are there situations other than an incoming http request that trigger the outdated check?
(In reply to comment #39) What you wrote is totally correct. I don't want to confuse the issue so I'll shut up now. :)
Created attachment 17955 [details] Compiler.java diff
Created attachment 17956 [details] Generator.java diff
Created attachment 17957 [details] JspSourceDependent.java diff
Created attachment 17958 [details] JspServletWrapper.java diff
I uploaded the diffs for the 4 changed files. This fixes the problem completely and addresses any and all concerns brought forth in the comments so far. What happens next?
Anyone? Bueller?
http://jira.jboss.com/jira/browse/JBAS-3081?page=all JBoss was one of the last standouts, it was relying on Jasper to decide to recompile or not. No more. Now everything gets recompiled on every redeploy. At least we can count on the correctness of its behavior now. Too bad for the performance that according to some comments here was of such paramount concern. Its also too bad that even with patches charitably submitted, that bugs can't or won't be fixed in Jasper.
(In reply to comment #47) > Its also too bad that even with patches charitably submitted, that bugs can't or > won't be fixed in Jasper. As I said earlier, I don't think fixing the edge cases is worth adding the related complexity. It's very simple. As for JBoss "fixing" it, Tomcat "fixes" it too: you simply need to undeploy/redeploy webapps, and/or add some listener to clean up the work directory, which is trivial to do. If that's all you want to achieve, why did you focus on a complex patch to Jasper ? BTW, feel free to post more useless rants, esp in conjunction with Gili, I enjoy them :)
(In reply to comment #48) > As I said earlier, I don't think fixing the edge cases is worth adding the > related complexity. It's very simple. As for JBoss "fixing" it, Tomcat "fixes" > it too: you simply need to undeploy/redeploy webapps, and/or add some listener > to clean up the work directory, which is trivial to do. If that's all you want > to achieve, why did you focus on a complex patch to Jasper ? The problem isn't isolated to an "edge" case, it affects the standard way apps are deployed. Thus the numerous other people who have encountered it. As for my patch being "complex" that's just not true. I added 2 fields to the generated servlet class and updated the isOutDated logic to use them. It took all of an hour to code and test. I can't believe you're asking me why I wanted to fix something that is broken rather than bandaid it.
> The problem isn't isolated to an "edge" case, it affects the standard way apps > are deployed. Thus the numerous other people who have encountered it. I certainly agree. I encountered this issue multiple times. I took quite some time and flustration to discover this bug. The workaround I currently use is deploying exploded .war contents and setting scp not to preserve timestamps, effectively forcing all copied JSP files to be recompiled. I think the bug is obviously serious and is definately not isolated to an "edge" case. I'm glad someone is trying to fix it.
(In reply to comment #50) > > The problem isn't isolated to an "edge" case, it affects the standard way apps > > are deployed. Thus the numerous other people who have encountered it. > > I certainly agree. I encountered this issue multiple times. I took quite some > time and flustration to discover this bug. > The workaround I currently use is deploying exploded .war contents and setting > scp not to preserve timestamps, effectively forcing all copied JSP files to be > recompiled. > I think the bug is obviously serious and is definately not isolated to an "edge" > case. > I'm glad someone is trying to fix it. I've fixed it, getting the fix incorporated into the codebase appears to be the impossible part. Feel free to use the code from my patch and give feedback if you find any problems.
(In reply to comment #51) > I've fixed it, getting the fix incorporated into the codebase appears to be the > impossible part. Feel free to use the code from my patch and give feedback if > you find any problems. Thanks, but it seems we will be migrating to JBoss 4.0.4 when the GA version comes out and it shouldn't be an issue anymore. I'm fortunate enough I don't have to use Tomcat standalone.
(In reply to comment #51) > I've fixed it, getting the fix incorporated into the codebase appears to be the > impossible part. Feel free to use the code from my patch and give feedback if > you find any problems. I just wasted a embarrasing amount of time trying to figure out why one of our developers was having problems seeing her changes 'live' on the test machine after changing a JSP file. I'd assumed that since I had Tomcat running in Development mode, it was going to recompile the page when it changed.... I'm glad to see that the seriousness of this bug has been recognized, and a patch developed... even if it doesn't seem to have the blessing of some of the developers. The patch certainly saved me a lot of hassle. Thanks!!
*** Bug 40420 has been marked as a duplicate of this bug. ***
I think this is a significant problem when you really have different organizations building and deploying, e.g., for publicly distributed applications. A common timeline would be: January - release 1.0 of app March - JSP in app is updated April - release 1.1 of app frozen for QA May - user downloads and installs 1.0 June - user downloads and installs 1.1 In this case, Jasper will view the compiled date of the 1.0 JSP as May and view it as newer than the change date of the 1.1 JSP. I gather that the "best practice" for building a web app for use with Tomcat would be to touch all the JSP's in your web app in your release process so you minimize the risk (although if someone downloads 1.0 after you released 1.1 they can expect a lot of 500 errors from NoSuchMethodError) I believe Jasper really should remove cached JSP's from the work directory when an app is undeployed (or redeployed). I think this is common, important, and quite different than the rather obscure case of rolling back an older version from version control. This would also avoid a performance hit in checking out of date on JSP's and wouldn't surprise people (I wouldn't expect JSP's to be cached after redeploying an app, indeed I think it's surprising behavior!) One of the users of our Web app just hit this issue today: http://www.glassbox.com/forum/forum/addpost?parent=235 and with a little googling you can see others e.g., http://mail-archives.apache.org/mod_mbox/tomcat-users/200512.mbox/%3C43A0096A.3060200@mkodo.com%3E
For the next release of your software, I would register an error handler that catches this error, and sends an email with the contents to the tomcat-dev mailing list. (In reply to comment #55) > if someone downloads 1.0 after you released 1.1 they can expect a lot of 500 > errors from NoSuchMethodError)
Why not just distribute your app with pre-compilied JSPs and avoid all these problems?
Our app needs to be portable to a variety of Servlet containers (and for different versions), so we can't precompile for any one server.
(In reply to comment #55) > I believe Jasper really should remove cached JSP's from the work directory when > an app is undeployed (or redeployed). I think this is common, important, and > quite different than the rather obscure case of rolling back an older version > from version control. This would also avoid a performance hit in checking out of > date on JSP's and wouldn't surprise people (I wouldn't expect JSP's to be cached > after redeploying an app, indeed I think it's surprising behavior!) This is a very good point that I would agree with. It is not upto any external tools to effectively manage the "work/" directory for Tomcat. This directory should be self-managing and be implemented on the side of caution, the caching of JSP pages is a benifit not a right. I think the following new rule would work: * The work/ directory is only cleaned of unused contexts when a web-app is undeployed (while the container is running, aka hot-undeploy) or found to no longer exist after all configuration parsing has been done at container startup. Although it is somewhat difficult to manage web-app upgrades taking place when the container is shutdown. Which I'd say was a pretty common event. One way around that situation would be to detect a web-app update has taken place. The simplest for TC (and the sys-admin) way I can think of, is for TC to remember the exact timestamp on the WEB-INF/web.xml file which the pre-compiled pages relate to. Make it create an empty file and touch up the timestamp to match the real web.xml as work/web.xml.timestamp. The sys-admin must then only touch the WEB-INF/web.xml (when he upgrades his web-app while the container is stopped). When TC boots up again it detects the timestamp is not equal and presumes the web-app was changed also, this causes a flush of the work/ for that context. The idea being this approach would be a whole lot simpler than re-validating the entire work/ cache from the source JSPs during all webapp deployments. It would be nice to have re-validation maybe that could be implemented using magic .java file comments in the japser output "// Jasper-JSP-Prerequisite: foobar/test.jsp 72383828372000" where both the top level source and all included fragments are listed with their Epoch millis for timestamp. Then the process would be to recurse the work/org/apache/ tree, reading all the .java and performing a simple stat() on the source files. This could be done in the background with live requests taking priority to be checked on first access after deployment. When work/* file that have been sucessfully revalidated (or deleted/recreated) have their timestamp updated, so that it is possible for any thread to know if a re-validation is required on a page, since the timestamp will be older than the deployment time of the web-app context. This would have no longterm JVM impact that loading classes might have, we can put what we like in the .java file and access it easily.
Daryl, see comments #35 - #45 and the 4 patch files I posted in the Attachment section. The patch I submitted in March completely fixes this problem in a manner similar to your suggestion. The patch works by storing the timestamp of the .jsp and all its dependent .jsps as member data in the compiled servlet. The isOutDated() method was modified to compare the timestamp of the .jsp against the added timestamps using != instead of >. (In reply to comment #59) > It would be nice to have re-validation maybe that could be implemented using > magic .java file comments in the japser output "// Jasper-JSP-Prerequisite: > foobar/test.jsp 72383828372000" where both the top level source and all included > fragments are listed with their Epoch millis for timestamp. Then the process > would be to recurse the work/org/apache/ tree, reading all the .java and > performing a simple stat() on the source files. This could be done in the > background with live requests taking priority to be checked on first access > after deployment.
What are the side-effects of revalidating the entire tree ? Does it cause all class files to be loaded or can the revalidation occur without having any lasting overheads (like increased memory consumption and slower revalidation process due to parsing of more complex .class data). My method only seeks to delete stale work/ .java and .class files during web-app deployment. It does not seek to recreate and load them, that can be left to moment of first use (although it would natually lead on to facilitating an automatic recreation function). By opening the .java file and looking for a magic comment and closing the file again, there is no lasting overhead. Since we never loaded the class. Which is just great for a revalidation pass during deployment. I'm a believer there should be a configuration mode of TC which is watertight, such that no amount of abuse will make the things fail in a way that bites you. The work/ directory is a nice speedup but the implementation is more a hack than an optimization, since it clearly breaks down in situations you wouldn't expect. This bug/problem hits developers a lot more than production upgrades.
My patch doesn't change the overall strategy for invoking the isOutDated() check, as you are suggesting. The isOutDated() check does load the class, as it did prior to my patch. Revalidating the entire tree would cause all the class files to be loaded, which would be bad. However, with the isOutDated() method fixed, I see no reason to revalidate the entire tree. (In reply to comment #61) > What are the side-effects of revalidating the entire tree ? Does it cause all > class files to be loaded or can the revalidation occur without having any > lasting overheads (like increased memory consumption and slower revalidation > process due to parsing of more complex .class data). > My method only seeks to delete stale work/ .java and .class files during web- app > deployment. > It does not seek to recreate and load them, that can be left to moment of first > use (although it would natually lead on to facilitating an automatic recreation > function). > By opening the .java file and looking for a magic comment and closing the file > again, there is no lasting overhead. Since we never loaded the class. Which is > just great for a revalidation pass during deployment. > I'm a believer there should be a configuration mode of TC which is watertight, > such that no amount of abuse will make the things fail in a way that bites you. > The work/ directory is a nice speedup but the implementation is more a hack > than an optimization, since it clearly breaks down in situations you wouldn't > expect. > This bug/problem hits developers a lot more than production upgrades.
(In reply to comment #62) Darryl's last comments aside on changing the management of the entire work tree, I want to ask Jonathan and anyone who's used his patches: have they been stable and OK? Have there been any modifications needed to them? If not, i.e. if they've been stable, I'm tempted to add them to the 5.5 tree.
I haven't had to make any additional changes to the code in the patch. I have only used the patch in conjunction with JBoss 4.0.2. I have been using the code in development and production since I posted it. (In reply to comment #63) > (In reply to comment #62) > Darryl's last comments aside on changing the management of the entire work tree, > I want to ask Jonathan and anyone who's used his patches: have they been stable > and OK? Have there been any modifications needed to them? If not, i.e. if > they've been stable, I'm tempted to add them to the 5.5 tree.
I just got bit by this bug and lost a good part of my day trying to locate it. I have tomcat on my development machine (winXP) configured to UTC and the machine itself is set for PST. Here's the procedure * make a change to my source jsp * war it up * stop tomcat * delete the existing war and webapps folder * copy the new war into the webapps dir * start tomcat The problem: Tomcat will expand the war however the last modified date on the jsp file will be adjusted (incorrectly?) to 8 hrs earlier. As the previous change was compiled less than 8hrs ago the jsp is not recompiled and the outdated .java and .class files remain in the work\Catalina\localhost directory. The result is my changes are not reflected. For now I will delete the work directory at the same time I delete the old war file. This way at least, as a developer, I know that what tomcat is serving is my most recent code.
*** Bug 15417 has been marked as a duplicate of this bug. ***
Created attachment 27040 [details] Proposed patch for Tomcat 7 Having spent a little time on this, I am attaching a proposed patch for Tomcat 7. The patch breaks binary compatibility for compiled JSPs which I am not at all comfortable about. I have some ideas for a solution to that which I will be discussing on the dev list. If this issue is addressed in Tomcat 7, I don't see the fix being back-ported to earlier versions.
Feedback on proposed TC7 patch: http://tomcat.markmail.org/thread/mbjdpr4bvw6gzx62
This has been fixed in 7.0.x and will be included in 7.0.17 onwards. The fix was fairly invasive so it will not be back-ported to 6.0.x or 5.5.x.