Bug 62603 - Changes in tag files are not reflected in the rendered view or they end up with a java.lang.NoClassDefFoundError
Summary: Changes in tag files are not reflected in the rendered view or they end up wi...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Jasper (show other bugs)
Version: 8.5.24
Hardware: All All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-07 18:44 UTC by Jordi
Modified: 2018-08-13 10:34 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jordi 2018-08-07 18:44:55 UTC
We have an application that contains jsps and tag files, when we deploy jsp/tag changes instead of reloading the webapp entirely we change those in the webapps folder, and let jsp reloading process do the work for us. 

From time to time, we suffer from two different kind of errors:

1- java.lang.NoClassDefFoundError because the tag file used in a jsp is "not found", but the jsp and also the tag file are there (TOMCA_HOME/work/Catalina/localhost/ROOT/org/apache/jsp/...)

2- changes in a tag file are never reflected in the rendered view(jsp)


I've took some time to debug the jsp reloading code, and here are my thoughts

Jsp reloading process at the end is calling JspServlet.periodicEvent method which triggers the process by calling JspRuntimeContext's checkUnload and checkCompile methods

JspRuntimeContext's jsps attribute is the one that contains jsps and tag references that are going to be checked from time to time, depending on checkInterval and development config values, to see if a JSP page (and its dependent files) need to be recompiled.

JspRuntimeContext's jsps attribute is backed by a ConcurrentHashMap, so the insertion order MAY NOT be preserved when we iterate through the list of jsps and dependant files. Check JspRuntimeContext.checkCompile() method.

The order in which we iterate through the tags and jsps is relevant because when a change in a tag file is detected the .java and also .class files are generated and its JspServletWrapper.reload attribute is set to true, afterwards in the same JspRuntimeContext.checkCompile() loop, when the process detects that a jsp needs to be recompiled because is "dependant" on this tag file  TagFileProcessor.loadTagFile method will invoke the JspServletWrapper.loadTagFile method that belongs to the modified tag, and because its reload attribute was set to true JSpServletWrapper's tagHandler attribute will be loaded again. After this process the .java and .class file that belong to that jsp will be generated 

But what would happen if we :
- detect a change in the jsp first and generate the .java and .class files (jsp needs to be recompiled because of a change in one dependant tag). 
- receive a request that match that jsp which will set JspServletWrapper.reload attribute(set to true in the previous step) to false while still "pointing" to the old tag class
- detect the change in the tag file which will recreate .java and .class files

Then, depending on the order of the incoming request and the jsp detection process :
- tag changes are not reflected in the rendered view
- or even worse java.lang.NoClassDefFoundError

Sorry for this long description but it's not easy to explain :)
Comment 1 Mark Thomas 2018-08-08 14:46:28 UTC
Thanks for the clear description of a tricky issue.

I've been looking at potential solutions.

Having run through various scenarios my conclusion is that there is always a risk of problems if the reload flags are inconsistent across the web application. To avoid the potential problems, reloading needs to be prevented while the reload flags are being updated.

I see two approaches to this:

1. In JspSrvletWrapper.getServlet() assume reload == false while the loop in JspRuntimeContext.checkCompile() is processing.

2. In JspSrvletWrapper.getServlet(), if reload == true && the loop in checkCompile() is processing pause the current request until checkCompile() completes.

My main concern with approach 1 is that it is possible, with (un)lucky timing that, after a resource is modified requests for that resource arrive during a checkCompile() processing window meaning the updated version of the resource is not seen even if it was modified multiple checkInterval periods ago. I don't think this is intuitive and coudl lead to unexpected behaviour.

With approach 2 the main concern is the length of the pause between the request being requested and processing starting. This is primarily driven by the time it takes to complete the checkCompile() loop.

I am currently leaning towards approach 2 with some additional logging that will tell admins how long the chekcCompile() loop took to complete and how long individual requests were waiting.
Comment 2 Jordi 2018-08-08 19:19:48 UTC
Thanks for the feedback Mark.

What about this variant of number 1

- In JspSrvletWrapper.getServlet() assume reload == false while the loop in JspRuntimeContext.checkCompile() is processing
- Store all the JspServletWrappers affected by the previous step(in JspRuntimeContext ?)
- When JspRuntimeContext.checkCompile() is completed, iterate through that "list" and reload them by invoking JspServletWrapper.getServlet

I could send a PR/patch if you wish
Comment 3 Mark Thomas 2018-08-09 09:40:43 UTC
I like it. Thanks for the offer of a patch. Normally I'd say yes please but in this case this issue is blocking me getting the next set of releases out so I am going to start working on it immediately.

Once thing I did notice is that we'll also need to take account of firstTime to ensure we don;t skip compiling a new JSP because we are in the JspRuntimeContext.checkCompile() loop.
Comment 4 Jordi 2018-08-09 09:45:22 UTC
Agree, great, thanks Mark!
Comment 5 Mark Thomas 2018-08-09 13:49:21 UTC
Fixed in:
- trunk for 9.0.11 onwards
- 8.5.x for 8.5.33 onwards
- 7.0.x for 7.0.91 onwards

I've tested this with a debugger and I can force a problem (updated tag doe snot take effect) before the patch but not afterwards. Additional review / testing welcome.
Comment 6 Jordi 2018-08-10 10:24:36 UTC
I've reviewed the changes and everything looks good(I will test/debug it also next week)

Thanks
Comment 7 Jordi 2018-08-13 10:34:35 UTC
Unable to reproduce the error with 8.5.33

Thanks