diff --git a/webapp/src/edu/cornell/mannlib/vitro/webapp/freemarker/config/FreemarkerConfigurationImpl.java b/webapp/src/edu/cornell/mannlib/vitro/webapp/freemarker/config/FreemarkerConfigurationImpl.java index 617a09f6c..7d2c05c9d 100644 --- a/webapp/src/edu/cornell/mannlib/vitro/webapp/freemarker/config/FreemarkerConfigurationImpl.java +++ b/webapp/src/edu/cornell/mannlib/vitro/webapp/freemarker/config/FreemarkerConfigurationImpl.java @@ -3,6 +3,7 @@ package edu.cornell.mannlib.vitro.webapp.freemarker.config; import java.io.IOException; +import java.lang.ref.WeakReference; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -35,28 +36,52 @@ import freemarker.template.utility.DeepUnwrap; * Extend the Freemarker Configuration class to include some information that is * particular to the current request. * - * Takes advantage of the fact that each servlet request runs in a separate - * thread. Stores the request-based information in a ThreadLocal. Override any - * methods that should return that information instead of (or in addition to) - * the common info. + * A reference to the current request is not always available to the Freemarker + * Configuration, so we take advantage of the fact that each request runs in a + * separate thread, and store a reference to that request in a ThreadLocal + * object. + * + * Then, we override any methods that should return that request-based + * information instead of (or in addition to) the common info. * * Only the getters are overridden, not the setters. So if you call * setAllSharedVariables(), for example, it will have no effect on the * request-based information. + * + * Notice that the reference to the current request is actually stored through a + * WeakReference. This is because the ThreadLocal will not be cleared when the + * webapp is stopped, so none of the references from that ThreadLocal are + * eligible for garbage collection. If any of those references is an instance of + * a class that is loaded by the webapp, then the webapp ClassLoader is not + * eligible for garbage collection. This would be a huge memory leak. + * + * Thanks to the WeakReference, the request is eligible for garbage collection + * if nothing else refers to it. In theory, this means that the WeakReference + * could return a null, but if the garbage collector has taken the request, then + * who is invoking this object? */ public class FreemarkerConfigurationImpl extends Configuration { private static final Log log = LogFactory .getLog(FreemarkerConfigurationImpl.class); - private final ThreadLocal rbiRef = new ThreadLocal<>(); + private static final String ATTRIBUTE_NAME = RequestBasedInformation.class + .getName(); + + private final ThreadLocal> reqRef = new ThreadLocal<>(); void setRequestInfo(HttpServletRequest req) { - rbiRef.set(new RequestBasedInformation(req, this)); + reqRef.set(new WeakReference<>(req)); + req.setAttribute(ATTRIBUTE_NAME, new RequestBasedInformation(req, this)); + } + + private RequestBasedInformation getRequestInfo() { + HttpServletRequest req = reqRef.get().get(); + return (RequestBasedInformation) req.getAttribute(ATTRIBUTE_NAME); } @Override public Object getCustomAttribute(String name) { - Map attribs = rbiRef.get().getCustomAttributes(); + Map attribs = getRequestInfo().getCustomAttributes(); if (attribs.containsKey(name)) { return attribs.get(name); } else { @@ -66,13 +91,13 @@ public class FreemarkerConfigurationImpl extends Configuration { @Override public String[] getCustomAttributeNames() { - Set rbiNames = rbiRef.get().getCustomAttributes().keySet(); + Set rbiNames = getRequestInfo().getCustomAttributes().keySet(); return joinNames(rbiNames, super.getCustomAttributeNames()); } @Override public TemplateModel getSharedVariable(String name) { - Map vars = rbiRef.get().getSharedVariables(); + Map vars = getRequestInfo().getSharedVariables(); if (vars.containsKey(name)) { return vars.get(name); } else { @@ -82,7 +107,7 @@ public class FreemarkerConfigurationImpl extends Configuration { @Override public Set getSharedVariableNames() { - Set rbiNames = rbiRef.get().getSharedVariables().keySet(); + Set rbiNames = getRequestInfo().getSharedVariables().keySet(); @SuppressWarnings("unchecked") Set superNames = super.getSharedVariableNames(); @@ -94,7 +119,7 @@ public class FreemarkerConfigurationImpl extends Configuration { @Override public Locale getLocale() { - return rbiRef.get().getReq().getLocale(); + return getRequestInfo().getReq().getLocale(); } private String[] joinNames(Set nameSet, String[] nameArray) { diff --git a/webapp/src/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettings.java b/webapp/src/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettings.java index 169491372..f883281f9 100644 --- a/webapp/src/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettings.java +++ b/webapp/src/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettings.java @@ -179,7 +179,7 @@ public class DeveloperSettings { // The factory // ---------------------------------------------------------------------- - private static final String ATTRIBUTE_NAME = DeveloperSettings.class + protected static final String ATTRIBUTE_NAME = DeveloperSettings.class .getName(); public static DeveloperSettings getBean(HttpServletRequest req) { @@ -203,7 +203,7 @@ public class DeveloperSettings { private final Map settings = new EnumMap<>(Keys.class); - private DeveloperSettings(ServletContext ctx) { + protected DeveloperSettings(ServletContext ctx) { updateFromFile(ctx); } diff --git a/webapp/test/stubs/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettingsStub.java b/webapp/test/stubs/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettingsStub.java new file mode 100644 index 000000000..6e2871596 --- /dev/null +++ b/webapp/test/stubs/edu/cornell/mannlib/vitro/webapp/utils/developer/DeveloperSettingsStub.java @@ -0,0 +1,33 @@ +/* $This file is distributed under the terms of the license in /doc/license.txt$ */ + +package stubs.edu.cornell.mannlib.vitro.webapp.utils.developer; + +import javax.servlet.ServletContext; + +import edu.cornell.mannlib.vitro.webapp.utils.developer.DeveloperSettings; + +/** + * Do everything that a standard DeveloperSettings would do, except loading from + * a properties file. + * + * That way, we don't require ConfigurationProperties to find the Vitro home + * directory, so we don't throw errors if there is no ConfigurationProperties. + */ +public class DeveloperSettingsStub extends DeveloperSettings { + /** + * Factory method. Create the stub and set it into the ServletContext. + */ + public static void set(ServletContext ctx) { + ctx.setAttribute(ATTRIBUTE_NAME, new DeveloperSettingsStub(ctx)); + } + + protected DeveloperSettingsStub(ServletContext ctx) { + super(ctx); + } + + @Override + protected void updateFromFile(ServletContext ctx) { + // Don't bother. + } + +} diff --git a/webapp/test/stubs/javax/servlet/ServletContextStub.java b/webapp/test/stubs/javax/servlet/ServletContextStub.java index 46e592d60..eb1e5eab7 100644 --- a/webapp/test/stubs/javax/servlet/ServletContextStub.java +++ b/webapp/test/stubs/javax/servlet/ServletContextStub.java @@ -21,6 +21,8 @@ import javax.servlet.ServletException; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import stubs.edu.cornell.mannlib.vitro.webapp.utils.developer.DeveloperSettingsStub; + /** * A simple stand-in for the {@link ServletContext}, for use in unit tests. */ @@ -36,6 +38,11 @@ public class ServletContextStub implements ServletContext { private final Map mockResources = new HashMap(); private final Map realPaths = new HashMap(); + public ServletContextStub() { + // Assume that unit tests won't want to use Developer mode. + DeveloperSettingsStub.set(this); + } + public void setContextPath(String contextPath) { if (contextPath == null) { throw new NullPointerException("contextPath may not be null."); diff --git a/webapp/web/js/vitroUtils.js b/webapp/web/js/vitroUtils.js index e910efa74..10e5f564b 100644 --- a/webapp/web/js/vitroUtils.js +++ b/webapp/web/js/vitroUtils.js @@ -13,20 +13,16 @@ $(document).ready(function(){ jQuery('section#flash-message').css('display', 'none').fadeIn(1500); ///////////////////////////// - // Home search fiter + // Home search filter // Toggle filter select list var $searchFilterList = $('#filter-search-nav'); var $isFilterOpen = false; - console.log("Filter is open = " + $isFilterOpen); - $('a.filter-search').click(function(e) { e.preventDefault(); if (!$isFilterOpen) { - console.log("Filter is closed = " + $isFilterOpen); - //Change button filter state to selected //$(this).css('background','url(../../themes/vivo-cornell/images/filteredSearchActive.gif) no-repeat right top'); $(this).removeClass('filter-default'); @@ -37,7 +33,6 @@ $(document).ready(function(){ $isFilterOpen = true; - console.log("open"); } else { //Change button filter state to default //$('a.filter-search').css('background','url(../../themes/vivo-cornell/images/filteredSearch.gif) no-repeat right top'); @@ -49,7 +44,6 @@ $(document).ready(function(){ $isFilterOpen = false; - console.log("closed"); } }); @@ -63,7 +57,6 @@ $(document).ready(function(){ //Selected filter feedback $('.search-filter-selected').text(''); $('input[name="classgroup"]').val(''); - console.log("ALL"); } else { $('.search-filter-selected').text($(this).text()).fadeIn('slow'); @@ -110,6 +103,5 @@ $(document).ready(function(){ } - console.log("HIDE input value ") ; }); });