Re: svn commit: r573442 - in /ofbiz/trunk: applications/content/src/org/ofbiz/content/email/ framework/appserver/src/org/ofbiz/appservers/ framework/base/src/base/org/ofbiz/base/util/template/

[Adrian Crum]

David,

Thank you for fixing this! I checked the version of FreeMarkerWorker.java just prior to the
refactor, and it appears to me that the Reader being left open bug was there previously.

Btw, the if (Debug.verboseOn()) condition on line 249 is redundant - see line 244.

-Adrian

[hidden email] wrote:

> Author: jonesde
> Date: Thu Sep  6 21:01:25 2007
> New Revision: 573442
>
> URL: http://svn.apache.org/viewvc?rev=573442&view=rev
> Log:
> Fixed bugs in recently refactored FreeMarkerWorker: a Reader was always being created even if the template was cached, also the reader was never closed which means files stayed open until the garbage collector cleaned up these objects which on busy production systems could lead to many thousands of open files
>
> Modified:
>     ofbiz/trunk/applications/content/src/org/ofbiz/content/email/NotificationServices.java
>     ofbiz/trunk/framework/appserver/src/org/ofbiz/appservers/GenerateContainer.java
>     ofbiz/trunk/framework/base/src/base/org/ofbiz/base/util/template/FreeMarkerWorker.java
>
> Modified: ofbiz/trunk/applications/content/src/org/ofbiz/content/email/NotificationServices.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/content/src/org/ofbiz/content/email/NotificationServices.java?rev=573442&r1=573441&r2=573442&view=diff
> ==============================================================================
> --- ofbiz/trunk/applications/content/src/org/ofbiz/content/email/NotificationServices.java (original)
> +++ ofbiz/trunk/applications/content/src/org/ofbiz/content/email/NotificationServices.java Thu Sep  6 21:01:25 2007
> @@ -205,12 +205,10 @@
>                  return ServiceUtil.returnError("Problem finding template; see logs");
>              }
>                                      
> -            InputStreamReader templateReader = new InputStreamReader(templateUrl.openStream());
> -                        
>              // process the template with the given data and write
>              // the email body to the String buffer
>              Writer writer = new StringWriter();
> -            FreeMarkerWorker.renderTemplate(templateName, templateReader, templateData, writer);
> +            FreeMarkerWorker.renderTemplate(templateUrl.toExternalForm(), templateData, writer);
>  
>              // extract the newly created body for the notification email
>              String notificationBody = writer.toString();            
>
> Modified: ofbiz/trunk/framework/appserver/src/org/ofbiz/appservers/GenerateContainer.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/appserver/src/org/ofbiz/appservers/GenerateContainer.java?rev=573442&r1=573441&r2=573442&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/appserver/src/org/ofbiz/appservers/GenerateContainer.java (original)
> +++ ofbiz/trunk/framework/appserver/src/org/ofbiz/appservers/GenerateContainer.java Thu Sep  6 21:01:25 2007
> @@ -29,6 +29,7 @@
>  import org.ofbiz.base.container.Container;
>  import org.ofbiz.base.container.ContainerException;
>  import org.ofbiz.base.util.Debug;
> +import org.ofbiz.base.util.UtilURL;
>  import org.ofbiz.base.util.template.FreeMarkerWorker;
>  import org.ofbiz.base.start.Classpath;
>  import org.ofbiz.base.component.ComponentConfig;
> @@ -141,12 +142,6 @@
>  
>      private void parseTemplate(File templateFile, Map dataMap) throws ContainerException {
>          Debug.log("Parsing template : " + templateFile.getAbsolutePath(), module);
> -        Reader reader = null;
> -        try {
> -            reader = new InputStreamReader(new FileInputStream(templateFile));
> -        } catch (FileNotFoundException e) {
> -            throw new ContainerException(e);
> -        }
>  
>          // create the target file/directory
>          String targetDirectoryName = args.length > 1 ? args[1] : null;
> @@ -174,7 +169,7 @@
>              throw new ContainerException(e);
>          }
>          try {
> -            FreeMarkerWorker.renderTemplate(templateFile.getAbsolutePath(), reader, dataMap, writer);
> +            FreeMarkerWorker.renderTemplate(UtilURL.fromFilename(templateFile.getAbsolutePath()).toExternalForm(), dataMap, writer);
>          } catch (Exception e) {
>              throw new ContainerException(e);
>          }
>
> Modified: ofbiz/trunk/framework/base/src/base/org/ofbiz/base/util/template/FreeMarkerWorker.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/base/org/ofbiz/base/util/template/FreeMarkerWorker.java?rev=573442&r1=573441&r2=573442&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/base/src/base/org/ofbiz/base/util/template/FreeMarkerWorker.java (original)
> +++ ofbiz/trunk/framework/base/src/base/org/ofbiz/base/util/template/FreeMarkerWorker.java Thu Sep  6 21:01:25 2007
> @@ -19,6 +19,7 @@
>  package org.ofbiz.base.util.template;
>  
>  import java.io.IOException;
> +import java.io.InputStream;
>  import java.io.InputStreamReader;
>  import java.io.Reader;
>  import java.io.StringReader;
> @@ -128,7 +129,7 @@
>       * @param outWriter The Writer to render to
>       */
>      public static void renderTemplateAtLocation(String templateLocation, Map context, Writer outWriter) throws MalformedURLException, TemplateException, IOException {
> -        renderTemplate(templateLocation, getTemplateReader(templateLocation), context, outWriter);
> +        renderTemplate(templateLocation, context, outWriter);
>      }
>      
>      /**
> @@ -138,9 +139,8 @@
>       * @param context The context Map
>       * @param outWriter The Writer to render to
>       */
> -    public static void renderTemplate(String templateId, String templateString, Map context, Writer outWriter) throws TemplateException, IOException {
> -        Reader templateReader = new StringReader(templateString);
> -        renderTemplate(templateId, templateReader, context, outWriter);
> +    public static void renderTemplate(String templateLocation, String templateString, Map context, Writer outWriter) throws TemplateException, IOException {
> +        renderTemplate(templateLocation, context, outWriter);
>      }
>      
>      /**
> @@ -150,8 +150,8 @@
>       * @param context The context Map
>       * @param outWriter The Writer to render to
>       */
> -    public static void renderTemplate(String templateId, Reader templateReader, Map context, Writer outWriter) throws TemplateException, IOException {
> -        Template template = getTemplate(templateId, templateReader);
> +    public static void renderTemplate(String templateLocation, Map context, Writer outWriter) throws TemplateException, IOException {
> +        Template template = getTemplate(templateLocation);
>          renderTemplate(template, context, outWriter);
>      }
>  
> @@ -220,8 +220,9 @@
>          }
>          return defaultOfbizConfig;
>      }
> -
> -    public static Reader getTemplateReader(String templateLocation) throws IOException {
> +    
> +    /** Make sure to close the reader when you're done! That's why this method is private, BTW. */
> +    private static Reader makeReader(String templateLocation) throws IOException {
>          if (UtilValidate.isEmpty(templateLocation)) {
>              throw new IllegalArgumentException("FreeMarker template location null or empty");
>          }
> @@ -229,14 +230,15 @@
>          URL locationUrl = null;
>          try {
>              locationUrl = FlexibleLocation.resolveLocation(templateLocation);
> -        }
> -        catch (MalformedURLException e) {
> +        } catch (MalformedURLException e) {
>              throw new IllegalArgumentException(e.getMessage());
>          }
>          if (locationUrl == null) {
>              throw new IllegalArgumentException("FreeMarker file not found at location: " + templateLocation);
>          }
> -        Reader templateReader = new InputStreamReader(locationUrl.openStream());
> +        
> +        InputStream locationIs = locationUrl.openStream();
> +        Reader templateReader = new InputStreamReader(locationIs);
>          
>          String locationProtocol = locationUrl.getProtocol();
>          if ("file".equals(locationProtocol) && Debug.verboseOn()) {
> @@ -244,35 +246,28 @@
>              int lastSlash = locationFile.lastIndexOf("/");
>              String locationDir = locationFile.substring(0, lastSlash);
>              String filename = locationFile.substring(lastSlash + 1);
> -            Debug.logVerbose("FreeMarker render: filename=" + filename + ", locationDir=" + locationDir, module);
> +            if (Debug.verboseOn()) Debug.logVerbose("FreeMarker render: filename=" + filename + ", locationDir=" + locationDir, module);
>          }
>          
>          return templateReader;
>      }
> -    
> +
>      /**
>       * Gets a Template instance from the template cache. If the Template instance isn't
>       * found in the cache, then one will be created.
>       * @param templateLocation Location of the template - file path or URL
>       */
>      public static Template getTemplate(String templateLocation) throws TemplateException, IOException {
> -        return getTemplate(templateLocation, getTemplateReader(templateLocation));
> -    }
> -    
> -    /**
> -     * Gets a Template instance from the template cache. If the Template instance isn't
> -     * found in the cache, then one will be created.
> -     * @param templateId A unique ID for this template
> -     * @param templateReader The Reader that reads the template
> -     */
> -    public static Template getTemplate(String templateId, Reader templateReader) throws TemplateException, IOException {
> -        Template template = (Template) cachedTemplates.get(templateId);
> +        Template template = (Template) cachedTemplates.get(templateLocation);
>          if (template == null) {
>              synchronized (cachedTemplates) {
> -                template = (Template) cachedTemplates.get(templateId);
> +                template = (Template) cachedTemplates.get(templateLocation);
>                  if (template == null) {
> -                    template = new Template(templateId, templateReader, getDefaultOfbizConfig());
> -                    cachedTemplates.put(templateId, template);
> +                    // only make the reader if we need it, and then close it right after!
> +                    Reader templateReader = makeReader(templateLocation);
> +                    template = new Template(templateLocation, templateReader, getDefaultOfbizConfig());
> +                    templateReader.close();
> +                    cachedTemplates.put(templateLocation, template);
>                  }
>              }
>          }
> @@ -612,12 +607,12 @@
>              return new FlexibleTemplateSource(name);
>          }
>          public long getLastModified(Object templateSource) {
> -            FlexibleTemplateSource fts = (FlexibleTemplateSource)templateSource;
> +            FlexibleTemplateSource fts = (FlexibleTemplateSource) templateSource;
>              return fts.getLastModified();
>          }
>          public Reader getReader(Object templateSource, String encoding) throws IOException {
> -            FlexibleTemplateSource fts = (FlexibleTemplateSource)templateSource;
> -            return getTemplateReader(fts.getTemplateLocation());
> +            FlexibleTemplateSource fts = (FlexibleTemplateSource) templateSource;
> +            return makeReader(fts.getTemplateLocation());
>          }
>          public void closeTemplateSource(Object templateSource) throws IOException {
>              // do nothing
>
>
>