diff --git a/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/DotResourceLoader.java b/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/DotResourceLoader.java index 136157492bf0..284d7bbe0369 100644 --- a/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/DotResourceLoader.java +++ b/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/DotResourceLoader.java @@ -5,7 +5,10 @@ import com.dotmarketing.util.Config; import com.dotmarketing.util.Logger; import com.dotmarketing.util.UtilMethods; +import com.google.common.util.concurrent.Striped; import java.io.InputStream; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.Lock; import org.apache.commons.collections.ExtendedProperties; import org.apache.velocity.exception.ResourceNotFoundException; import org.apache.velocity.runtime.resource.Resource; @@ -13,10 +16,33 @@ public class DotResourceLoader extends ResourceLoader { - private static DotResourceLoader instance; - private static boolean useCache = Config.getBooleanProperty("VELOCITY_CACHING_ON", true); + private static final boolean useCache = Config.getBooleanProperty("VELOCITY_CACHING_ON", true); + + /** + * Timeout in milliseconds for acquiring the resource generation lock. + * If a resource takes longer than this to generate, waiting threads will timeout. + * Configurable via VELOCITY_RESOURCE_LOAD_TIMEOUT_MS property. + */ + private static final long RESOURCE_LOAD_TIMEOUT_MS = + Config.getLongProperty("VELOCITY_RESOURCE_LOAD_TIMEOUT_MS", 15000L); // 15 seconds default + + /** + * Number of lock stripes for resource generation. + * Higher values reduce contention but use more memory. + * Configurable via VELOCITY_RESOURCE_LOAD_STRIPES property. + */ + private static final int RESOURCE_LOAD_STRIPES = + Config.getIntProperty("VELOCITY_RESOURCE_LOAD_STRIPES", 64); + + /** + * Striped lock to prevent duplicate resource generation. + * Only one thread can generate a resource for a given key at a time. + * Other threads waiting for the same key will wait until generation completes or timeout. + */ + private static final Striped resourceLock = Striped.lazyWeakLock(RESOURCE_LOAD_STRIPES); + public DotResourceLoader() { super(); } @@ -28,58 +54,75 @@ public InputStream getResourceStream(final String filePath) throws ResourceNotFo throw new ResourceNotFoundException("cannot find resource"); } + final VelocityResourceKey key = new VelocityResourceKey(filePath); + Logger.debug(this, "DotResourceLoader:\t: " + key); - synchronized (filePath.intern()) { - - VelocityResourceKey key = new VelocityResourceKey(filePath); - - Logger.debug(this, "DotResourceLoader:\t: " + key); - - try { - switch (key.type) { - case CONTAINER: { - return new ContainerLoader().writeObject(key); - } - case TEMPLATE: { - return new TemplateLoader().writeObject(key); - } - case CONTENT: { - return new ContentletLoader().writeObject(key); - } - case FIELD: { - return new FieldLoader().writeObject(key); - } - case CONTENT_TYPE: { - return new ContentTypeLoader().writeObject(key); - } - case SITE: { - return new SiteLoader().writeObject(key); - } - case HTMLPAGE: { - return new PageLoader().writeObject(key); - } - case VELOCITY_MACROS: { - return VTLLoader.instance().writeObject(key); - } - case VTL: { - return VTLLoader.instance().writeObject(key); - } - case VELOCITY_LEGACY_VL: { - return VTLLoader.instance().writeObject(key); - } - default: { - return IncludeLoader.instance().writeObject(key); - } - } + // Use striped lock to prevent duplicate resource generation for the same key. + // This prevents the "thundering herd" problem where multiple threads try to + // generate the same expensive resource simultaneously. + final Lock lock = resourceLock.get(key.path); + boolean hasLock = false; + + try { + hasLock = lock.tryLock(RESOURCE_LOAD_TIMEOUT_MS, TimeUnit.MILLISECONDS); + + if (!hasLock) { + Logger.warn(this, "Timeout waiting for resource generation lock: " + key.path + + " after " + RESOURCE_LOAD_TIMEOUT_MS + "ms"); + throw new ResourceNotFoundException( + "Timeout waiting for velocity resource generation: " + key.path); + } - } catch (Exception e) { - Logger.warn(this, "filePath: " + filePath + ", msg:" + e.getMessage(), e); - CacheLocator.getVeloctyResourceCache().addMiss(key.path); - throw new ResourceNotFoundException("Cannot parse velocity file : " + key.path, e); + return loadResource(key); + + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new ResourceNotFoundException("Interrupted while waiting for resource: " + key.path, e); + } catch (ResourceNotFoundException e) { + throw e; + } catch (Exception e) { + Logger.warn(this, "filePath: " + filePath + ", msg:" + e.getMessage(), e); + CacheLocator.getVeloctyResourceCache().addMiss(key.path); + throw new ResourceNotFoundException("Cannot parse velocity file : " + key.path, e); + } finally { + if (hasLock) { + lock.unlock(); } } + } + /** + * Loads the velocity resource based on its type. + * This method is called while holding the resource lock to prevent duplicate generation. + * + * @param key the velocity resource key + * @return the resource as an InputStream + * @throws Exception if resource loading fails + */ + private InputStream loadResource(final VelocityResourceKey key) throws Exception { + switch (key.type) { + case CONTAINER: + return new ContainerLoader().writeObject(key); + case TEMPLATE: + return new TemplateLoader().writeObject(key); + case CONTENT: + return new ContentletLoader().writeObject(key); + case FIELD: + return new FieldLoader().writeObject(key); + case CONTENT_TYPE: + return new ContentTypeLoader().writeObject(key); + case SITE: + return new SiteLoader().writeObject(key); + case HTMLPAGE: + return new PageLoader().writeObject(key); + case VELOCITY_MACROS: + case VTL: + case VELOCITY_LEGACY_VL: + return VTLLoader.instance().writeObject(key); + default: + return IncludeLoader.instance().writeObject(key); + } } /* diff --git a/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/PageRenderUtil.java b/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/PageRenderUtil.java index 0b06a8db46fb..4b74c11e180e 100644 --- a/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/PageRenderUtil.java +++ b/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/PageRenderUtil.java @@ -59,7 +59,6 @@ import com.liferay.util.StringPool; import io.vavr.control.Try; import java.io.Serializable; -import java.io.StringWriter; import java.text.SimpleDateFormat; import java.time.Instant; import java.util.Collection; @@ -83,6 +82,14 @@ public class PageRenderUtil implements Serializable { public static String CONTAINER_UUID_PREFIX = "uuid-"; private static final long serialVersionUID = 1L; + /** + * ThreadLocal StringBuilder to avoid allocating new StringBuilder/StringWriter objects + * in the asString() method. This reduces GC pressure during page rendering as asString() + * is called for every page load to generate Velocity variable assignments. + */ + private static final ThreadLocal STRING_BUILDER = + ThreadLocal.withInitial(() -> new StringBuilder(2048)); + private final PermissionAPI permissionAPI = APILocator.getPermissionAPI(); private final MultiTreeAPI multiTreeAPI = APILocator.getMultiTreeAPI(); private final ContentletAPI contentletAPI = APILocator.getContentletAPI(); @@ -746,18 +753,24 @@ public Context addAll(Context incoming) { /** * Return the Velocity code to set the Page's variables as: contentletList, contentType, * totalSize, etc. + *

+ * Uses a ThreadLocal StringBuilder to minimize object allocation during page rendering. * - * @return + * @return The Velocity code as a String */ public String asString() { - - final StringWriter s = new StringWriter(); - for (String key : this.contextMap.keySet()) { - s.append("#set($").append(key.replace(":", "")).append("=").append(new StringifyObject(this.contextMap.get(key)).from()).append(')'); + final StringBuilder sb = STRING_BUILDER.get(); + sb.setLength(0); // Reset without reallocation + + for (final Map.Entry entry : this.contextMap.entrySet()) { + sb.append("#set($") + .append(entry.getKey().replace(":", "")) + .append('=') + .append(new StringifyObject(entry.getValue()).from()) + .append(')'); } - return s.toString(); - + return sb.toString(); } /** diff --git a/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/StringifyObject.java b/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/StringifyObject.java index 787524613bd5..e421bc3c4e32 100644 --- a/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/StringifyObject.java +++ b/dotCMS/src/main/java/com/dotcms/rendering/velocity/services/StringifyObject.java @@ -1,14 +1,32 @@ package com.dotcms.rendering.velocity.services; - -import java.io.StringWriter; import java.text.SimpleDateFormat; import java.util.Collection; import java.util.Date; import java.util.Iterator; +/** + * Utility class to convert objects to their string representation for Velocity context. + * Uses ThreadLocal buffers to minimize object allocation during page rendering. + */ final class StringifyObject { + + /** + * ThreadLocal StringBuilder pool to avoid allocating new StringWriter/StringBuilder + * for each stringify operation. This significantly reduces GC pressure during + * page rendering where many objects need to be stringified. + */ + private static final ThreadLocal BUFFER = + ThreadLocal.withInitial(() -> new StringBuilder(256)); + + /** + * ThreadLocal SimpleDateFormat to avoid creating expensive formatter objects. + * SimpleDateFormat is not thread-safe, so ThreadLocal is required. + */ + private static final ThreadLocal DATE_FORMAT = + ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS")); + final String stringified; public StringifyObject(final Object o) { @@ -37,62 +55,67 @@ else if(o instanceof String) { } + /** + * Gets the ThreadLocal StringBuilder, resetting it for reuse. + * @return A clean StringBuilder ready for use + */ + private static StringBuilder getBuffer() { + final StringBuilder sb = BUFFER.get(); + sb.setLength(0); // Reset without reallocation + return sb; + } + private String stringifyObject(final String[] str) { - StringWriter sw = new StringWriter(); - sw.append('['); + final StringBuilder sb = getBuffer(); + sb.append('['); for (int i = 0; i < str.length; i++) { - sw.append('"') + sb.append('"') .append(str[i]) - .append("\""); + .append('"'); if (i != str.length - 1) { - sw.append(","); + sb.append(','); } } - sw.append(']'); - return sw.toString(); + sb.append(']'); + return sb.toString(); } - private String stringifyObject(final Collection co) { - StringWriter sw = new StringWriter(); - - sw.append('['); - Iterator it = co.iterator(); + private String stringifyObject(final Collection co) { + final StringBuilder sb = getBuffer(); + sb.append('['); + final Iterator it = co.iterator(); while (it.hasNext()) { - Object obj = it.next(); - sw.append('"') + final Object obj = it.next(); + sb.append('"') .append(obj.toString()) - .append("\""); - - if(it.hasNext()) { - sw.append(","); + .append('"'); + if (it.hasNext()) { + sb.append(','); } } - sw.append(']'); - return sw.toString(); + sb.append(']'); + return sb.toString(); } - private String stringifyObject(final Boolean o) { - - return ((Boolean)o).toString(); + private String stringifyObject(final Boolean o) { + return o.toString(); } - private String stringifyObject(final String x) { - StringWriter sw = new StringWriter(); - - sw.append('"'); - sw.append(x.toString().replace("\"", "`")); - sw.append('"'); - return sw.toString(); - + private String stringifyObject(final String x) { + final StringBuilder sb = getBuffer(); + sb.append('"'); + sb.append(x.replace("\"", "`")); + sb.append('"'); + return sb.toString(); } - private String stringifyObject(final Date x) { - StringWriter sw = new StringWriter(); - String d = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSS").format(x); - sw.append('"'); - sw.append(d); - sw.append('"'); - return sw.toString(); + private String stringifyObject(final Date x) { + final StringBuilder sb = getBuffer(); + final String d = DATE_FORMAT.get().format(x); + sb.append('"'); + sb.append(d); + sb.append('"'); + return sb.toString(); } diff --git a/dotCMS/src/main/java/com/dotcms/rendering/velocity/servlet/VelocityLiveMode.java b/dotCMS/src/main/java/com/dotcms/rendering/velocity/servlet/VelocityLiveMode.java index cb3d9ef447b3..f65840adba9c 100644 --- a/dotCMS/src/main/java/com/dotcms/rendering/velocity/servlet/VelocityLiveMode.java +++ b/dotCMS/src/main/java/com/dotcms/rendering/velocity/servlet/VelocityLiveMode.java @@ -55,14 +55,63 @@ public class VelocityLiveMode extends VelocityModeHandler { - final static ThreadLocal byteArrayLocal = ThreadLocal.withInitial( - ByteArrayOutputStream::new); - final static long PAGE_CACHE_TIMEOUT_MILLIS = Config.getIntProperty("PAGE_CACHE_TIMEOUT_MILLIS", 2000); + /** + * Initial capacity for the page rendering buffer. + * Most pages are under 64KB, so this avoids early resizing. + * Configurable via PAGE_BUFFER_INITIAL_SIZE property. + */ + private static final int BUFFER_INITIAL_SIZE = + Config.getIntProperty("PAGE_BUFFER_INITIAL_SIZE", 64 * 1024); // 64KB default + + /** + * Maximum retained capacity for the ThreadLocal buffer. + * If a page grows the buffer beyond this size, we replace it with a fresh one + * after the request completes to prevent memory bloat from large pages. + * Configurable via PAGE_BUFFER_MAX_RETAINED_SIZE property. + */ + private static final int BUFFER_MAX_RETAINED_SIZE = + Config.getIntProperty("PAGE_BUFFER_MAX_RETAINED_SIZE", 256 * 1024); // 256KB default + + /** + * ThreadLocal buffer for capturing page output during cache population. + * Uses a custom ResettableByteArrayOutputStream that exposes capacity info. + */ + private static final ThreadLocal byteArrayLocal = + ThreadLocal.withInitial(() -> new ResettableByteArrayOutputStream(BUFFER_INITIAL_SIZE)); + private static final long PAGE_CACHE_TIMEOUT_MILLIS = Config.getIntProperty("PAGE_CACHE_TIMEOUT_MILLIS", 2000); private static final Striped stripedLock = Striped.lazyWeakLock( Config.getIntProperty("PAGE_CACHE_STRIPES", 64)); + /** + * A ByteArrayOutputStream that exposes its internal buffer capacity for memory management. + * This allows us to detect when the buffer has grown too large and should be replaced. + */ + static final class ResettableByteArrayOutputStream extends ByteArrayOutputStream { + + ResettableByteArrayOutputStream(final int initialCapacity) { + super(initialCapacity); + } + + /** + * Returns the current capacity of the internal buffer (not the data size). + * @return the buffer capacity in bytes + */ + int getCapacity() { + return buf.length; + } + + /** + * Checks if the buffer has grown beyond the specified threshold. + * @param threshold the maximum acceptable capacity + * @return true if buffer capacity exceeds threshold + */ + boolean exceedsCapacity(final int threshold) { + return buf.length > threshold; + } + } + @Deprecated public VelocityLiveMode(final HttpServletRequest request, final HttpServletResponse response, final String uri, @@ -155,10 +204,11 @@ public final void serve(final OutputStream out) throws DotDataException, IOExcep final PageCacheParameters cacheParameters = buildCacheParameters(langId, htmlPage); - String cachedPage = pageCache.get(htmlPage, cacheParameters); - if (cachedPage != null) { - // have cached response and are not refreshing, send it - out.write(cachedPage.getBytes(StandardCharsets.UTF_8)); + // Use byte[] cache to avoid String conversion overhead + byte[] cachedBytes = pageCache.getBytes(htmlPage, cacheParameters); + if (cachedBytes != null) { + // have cached response and are not refreshing, send it directly + out.write(cachedBytes); return; } @@ -168,22 +218,24 @@ public final void serve(final OutputStream out) throws DotDataException, IOExcep try { hasLock = lock.tryLock(PAGE_CACHE_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS); - String cachedPage2 = pageCache.get(htmlPage, cacheParameters); - if (cachedPage2 != null) { + // Double-check cache inside lock using byte[] for efficiency + byte[] cachedBytes2 = pageCache.getBytes(htmlPage, cacheParameters); + if (cachedBytes2 != null) { Logger.debug(this.getClass(), "Found cached page in striped lock: " + cacheParameters.getKey()); - // have cached response and are not refreshing, send it - out.write(cachedPage2.getBytes(StandardCharsets.UTF_8)); + // have cached response and are not refreshing, send it directly + out.write(cachedBytes2); return; } if (hasLock) { try (Writer tmpOut = new OutputStreamWriter(new TeeOutputStream(out, byteArrayLocal.get()))) { - int startResponse = response.getStatus(); + final int startResponse = response.getStatus(); writePage(tmpOut, htmlPage); tmpOut.flush(); // if velocity has not changed the response status, we can cache it if (response.getStatus() == startResponse) { - pageCache.add(htmlPage, byteArrayLocal.get().toString(StandardCharsets.UTF_8), cacheParameters); + // Cache bytes directly - avoids toString() String allocation + pageCache.addBytes(htmlPage, byteArrayLocal.get().toByteArray(), cacheParameters); } } } else { @@ -202,11 +254,31 @@ public final void serve(final OutputStream out) throws DotDataException, IOExcep if (hasLock) { lock.unlock(); } - byteArrayLocal.get().reset(); + resetBuffer(); } } + /** + * Resets the ThreadLocal buffer for reuse. + * If the buffer has grown beyond BUFFER_MAX_RETAINED_SIZE, it is replaced with a fresh one + * to prevent memory bloat from large pages persisting in the ThreadLocal. + */ + private static void resetBuffer() { + final ResettableByteArrayOutputStream buffer = byteArrayLocal.get(); + if (buffer.exceedsCapacity(BUFFER_MAX_RETAINED_SIZE)) { + // Buffer grew too large (e.g., from a very large page), replace it + // to release the oversized byte array back to GC + Logger.debug(VelocityLiveMode.class, () -> + "Page buffer exceeded max retained size (" + buffer.getCapacity() + + " > " + BUFFER_MAX_RETAINED_SIZE + "), replacing with fresh buffer"); + byteArrayLocal.set(new ResettableByteArrayOutputStream(BUFFER_INITIAL_SIZE)); + } else { + // Normal case: just reset the buffer position, keep the allocated array + buffer.reset(); + } + } + /** * Builds PageCacheParameters with all necessary cache keys for page caching. diff --git a/dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java b/dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java index 9cae3e26121f..ff01fa958d11 100644 --- a/dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java +++ b/dotCMS/src/main/java/com/dotcms/rendering/velocity/util/VelocityUtil.java @@ -212,21 +212,42 @@ public static Boolean isNotAllowedVelocityVariableName(String variable){ } /** - * Returns a basic Velocity Context without any toolbox or request - * response, session objects; - * @return + * Shared singleton instances of stateless utility classes. + * These are placed in every Velocity context but don't need to be recreated + * for each request since they contain no mutable state. + */ + private static final UtilMethods UTIL_METHODS_INSTANCE = new UtilMethods(); + private static final PortletURLUtil PORTLET_URL_UTIL_INSTANCE = new PortletURLUtil(); + private static final InodeUtils INODE_UTILS_INSTANCE = new InodeUtils(); + + /** + * Constant string values used in every context. + * Interned to ensure we use the same String instances. */ + private static final String QUOTE_VALUE = "\""; + private static final String POUNDS_VALUE = "##"; + private static final String RETURN_VALUE = "\n"; + private static final String DEFAULT_LANGUAGE = "1"; + /** + * Returns a basic Velocity Context without any toolbox or request + * response, session objects. + *

+ * Uses singleton instances of stateless utility classes to reduce + * object allocation during page rendering. + * + * @return a new VelocityContext with basic utilities pre-populated + */ public static Context getBasicContext() { - Context context = new VelocityContext(); - context.put("UtilMethods", new UtilMethods()); - context.put("PortletURLUtil", new PortletURLUtil()); - context.put("quote", "\""); - context.put("pounds", "##"); - context.put("return", "\n"); + final Context context = new VelocityContext(); + context.put("UtilMethods", UTIL_METHODS_INSTANCE); + context.put("PortletURLUtil", PORTLET_URL_UTIL_INSTANCE); + context.put("quote", QUOTE_VALUE); + context.put("pounds", POUNDS_VALUE); + context.put("return", RETURN_VALUE); context.put("velocityContext", context); - context.put("language", "1"); - context.put("InodeUtils", new InodeUtils()); + context.put("language", DEFAULT_LANGUAGE); + context.put("InodeUtils", INODE_UTILS_INSTANCE); return context; } diff --git a/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/content/ContentMap.java b/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/content/ContentMap.java index 57962aec28ce..0e711d29c89d 100644 --- a/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/content/ContentMap.java +++ b/dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/content/ContentMap.java @@ -75,6 +75,22 @@ */ public class ContentMap implements Serializable { + private static final long serialVersionUID = 1L; + + /** + * ThreadLocal StringBuilder for building tag lists without allocation per field access. + * Used when processing TAG field types. + */ + private static final ThreadLocal TAG_BUILDER = + ThreadLocal.withInitial(() -> new StringBuilder(256)); + + /** + * ThreadLocal StringWriter for velocity template merging without allocation per field access. + * Used when field values contain velocity code that needs parsing. + */ + private static final ThreadLocal VELOCITY_WRITER = + ThreadLocal.withInitial(() -> new StringWriter(1024)); + private Contentlet content; private ContentletAPI conAPI; private PermissionAPI perAPI; @@ -320,21 +336,18 @@ private Object get(final String fieldVariableName, final Boolean parseVelocity) } return null; }else if(f != null && f.getFieldType().equals(Field.FieldType.TAG.toString())){ - - StringBuilder tags = new StringBuilder(); + // Use ThreadLocal StringBuilder to avoid allocation per tag field access + final StringBuilder tags = TAG_BUILDER.get(); + tags.setLength(0); //Search for the list of tags related to this contentlet - List foundTags = APILocator.getTagAPI().getTagsByInode(content.getInode()); + final List foundTags = APILocator.getTagAPI().getTagsByInode(content.getInode()); if ( foundTags != null && !foundTags.isEmpty() ) { - - Iterator iterator = foundTags.iterator(); + final Iterator iterator = foundTags.iterator(); while ( iterator.hasNext() ) { - - Tag foundTag = iterator.next(); - tags.append(foundTag.getTagName()); - + tags.append(iterator.next().getTagName()); if ( iterator.hasNext() ) { - tags.append(","); + tags.append(','); } } } @@ -412,11 +425,12 @@ public String toString() { //handle Velocity Code if(parseVelocity && ret != null && (f == null || f.getFieldType().equals(Field.FieldType.TEXT.toString()) || f.getFieldType().equals(Field.FieldType.TEXT_AREA.toString()) || f.getFieldType().equals(Field.FieldType.CUSTOM_FIELD.toString()) || f.getFieldType().equals(Field.FieldType.WYSIWYG.toString())) && (ret.toString().contains("#") || ret.toString().contains("$"))){ - VelocityEngine ve = VelocityUtil.getEngine(); - Template template = null; - StringWriter sw = new StringWriter(); + // Use ThreadLocal StringWriter to avoid allocation per velocity field parse + final StringWriter sw = VELOCITY_WRITER.get(); + sw.getBuffer().setLength(0); - template = ve.getTemplate((EDIT_OR_PREVIEW_MODE ? PageMode.PREVIEW_MODE.name():PageMode.LIVE.name()) + File.separator + content.getInode() + File.separator + f.getInode() + "." + VelocityType.FIELD.fileExtension); + final VelocityEngine ve = VelocityUtil.getEngine(); + final Template template = ve.getTemplate((EDIT_OR_PREVIEW_MODE ? PageMode.PREVIEW_MODE.name():PageMode.LIVE.name()) + File.separator + content.getInode() + File.separator + f.getInode() + "." + VelocityType.FIELD.fileExtension); template.merge(context, sw); ret = sw.toString(); } diff --git a/dotCMS/src/main/java/com/dotmarketing/business/PageCacheParameters.java b/dotCMS/src/main/java/com/dotmarketing/business/PageCacheParameters.java index ff5aaa4ffa47..5cc8bc46c4f9 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/PageCacheParameters.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/PageCacheParameters.java @@ -6,7 +6,6 @@ import com.dotmarketing.util.UtilMethods; import io.vavr.Lazy; import java.util.Arrays; -import java.util.Set; import java.util.TreeSet; import java.util.concurrent.atomic.AtomicLong; @@ -20,6 +19,18 @@ */ public class PageCacheParameters { + /** + * ThreadLocal TreeSet for sorting query parameters without allocating a new Set per request. + * TreeSet provides natural ordering which ensures consistent cache key generation. + */ + private static final ThreadLocal> QUERY_SET_LOCAL = + ThreadLocal.withInitial(TreeSet::new); + + /** + * ThreadLocal StringBuilder for building filtered query strings without allocation per request. + */ + private static final ThreadLocal QUERY_BUILDER_LOCAL = + ThreadLocal.withInitial(() -> new StringBuilder(256)); static final AtomicLong expired = new AtomicLong(); static final long REFRESH_INTERVAL = 1000 * 60 * 5; // 5 minutes @@ -97,35 +108,48 @@ static boolean matches(String test, String[] params) { * normalizing (lowercasing) the values, sorting them alphabetically, * and determining which parameters to include or exclude * based on the "respect" or "ignore" parameters defined in the system. + *

+ * Uses ThreadLocal buffers to minimize object allocation per request. * * @param queryString the query string to be filtered; may include key-value pairs separated by `&` or `?` * @return a filtered and alphabetically sorted query string containing the selected parameters, * or {@code null} if the provided query string is empty */ - public static String filterQueryString(String queryString) { + public static String filterQueryString(final String queryString) { if (UtilMethods.isEmpty(queryString)) { return null; } - // sorts the query string parameters alphabetically - Set querySplit = new TreeSet<>(Arrays.asList(filterNulls(queryString.toLowerCase().split("[?&]", -1)))); - StringBuilder queryStringBuilder = new StringBuilder(); + + // Get and reset ThreadLocal buffers + final TreeSet querySplit = QUERY_SET_LOCAL.get(); + final StringBuilder queryStringBuilder = QUERY_BUILDER_LOCAL.get(); + querySplit.clear(); + queryStringBuilder.setLength(0); + + // Split, filter nulls, and add to TreeSet for sorting + final String[] parts = queryString.toLowerCase().split("[?&]", -1); + for (final String part : parts) { + if (UtilMethods.isSet(part)) { + querySplit.add(part); + } + } + + // Build filtered query string if (getRespectParams().length > 0) { - for (String queryParam : querySplit) { + for (final String queryParam : querySplit) { if (matches(queryParam, getRespectParams())) { - queryStringBuilder.append("&").append(queryParam); + queryStringBuilder.append('&').append(queryParam); } } - return queryStringBuilder.toString(); + return queryStringBuilder.length() > 0 ? queryStringBuilder.toString() : null; } - - for (String queryParam : querySplit) { + for (final String queryParam : querySplit) { if (!matches(queryParam, getIgnoreParams())) { - queryStringBuilder.append("&").append(queryParam); + queryStringBuilder.append('&').append(queryParam); } } return queryStringBuilder.length() > 0 ? queryStringBuilder.toString() : null; - } diff --git a/dotCMS/src/main/java/com/dotmarketing/business/StaticPageCache.java b/dotCMS/src/main/java/com/dotmarketing/business/StaticPageCache.java index 0b331f4017d9..6be40ec163ed 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/StaticPageCache.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/StaticPageCache.java @@ -3,10 +3,10 @@ import com.dotmarketing.portlets.htmlpageasset.model.IHTMLPage; /** - * This cache will keep a set of different {@link HTMLPage} objects and their - * different versions, i.e., the specific user ID, language, etc., used when + * This cache will keep a set of different {@link HTMLPage} objects and their + * different versions, i.e., the specific user ID, language, etc., used when * they were requested. - * + * * @author Jose Castro * @version 1.0 * @since 10-17-2014 @@ -25,32 +25,60 @@ public abstract class StaticPageCache implements Cachable { /** * Adds a new entry to the cache. - * + * * @param page * - The {@link IHTMLPage} object. * @param value * - The String representation of the page. - * @param pageChacheParams + * @param pageCacheParams * - Values used to cache a specific page. + * @deprecated Use {@link #addBytes(IHTMLPage, byte[], PageCacheParameters)} for better performance */ + @Deprecated abstract public void add(IHTMLPage page, String value, - PageCacheParameters pageChacheParams); + PageCacheParameters pageCacheParams); + + /** + * Adds a new entry to the cache using raw bytes. + * This is more efficient than {@link #add(IHTMLPage, String, PageCacheParameters)} + * as it avoids the byte[] → String → byte[] conversion roundtrip. + * + * @param page - The {@link IHTMLPage} object. + * @param content - The UTF-8 encoded byte array of the page content. + * @param pageCacheParams - Values used to cache a specific page. + */ + abstract public void addBytes(IHTMLPage page, byte[] content, + PageCacheParameters pageCacheParams); /** * Retrieves a page from the cache. - * + * * @param page * - The {@link IHTMLPage} object. - * @param pageChacheParams + * @param pageCacheParams * - Values used to retrieve a specific page from the cache. - * @return + * @return The cached page content as a String, or null if not found + * @deprecated Use {@link #getBytes(IHTMLPage, PageCacheParameters)} for better performance */ + @Deprecated abstract public String get(IHTMLPage page, - PageCacheParameters pageChacheParams); + PageCacheParameters pageCacheParams); + + /** + * Retrieves a page from the cache as raw bytes. + * This is more efficient than {@link #get(IHTMLPage, PageCacheParameters)} + * as it avoids creating intermediate String objects. + * + * @param page - The {@link IHTMLPage} object. + * @param pageCacheParams - Values used to retrieve a specific page from the cache. + * @return The cached page content as UTF-8 bytes, or null if not found + */ + abstract public byte[] getBytes(IHTMLPage page, + PageCacheParameters pageCacheParams); /** * Removes a page from the cache, along with all of its versions. - * + * * @param page * - The {@link IHTMLPage} object. */ diff --git a/dotCMS/src/main/java/com/dotmarketing/business/StaticPageCacheImpl.java b/dotCMS/src/main/java/com/dotmarketing/business/StaticPageCacheImpl.java index b62584cdc009..1446802f6f12 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/StaticPageCacheImpl.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/StaticPageCacheImpl.java @@ -5,6 +5,7 @@ import com.dotmarketing.portlets.htmlpageasset.model.IHTMLPage; import com.dotmarketing.util.Logger; import com.dotmarketing.util.UtilMethods; +import java.nio.charset.StandardCharsets; /** * Provides the caching implementation for HTML pages. This approach uses a main key to retrieve a cached page, and a @@ -66,6 +67,7 @@ public void clearCache() { } @Override + @Deprecated public void add(IHTMLPage page, final String pageContent, PageCacheParameters pageCacheParams) { if (UtilMethods.isEmpty(() -> page.getIdentifier()) || pageCacheParams == null) { return; @@ -81,8 +83,24 @@ public void add(IHTMLPage page, final String pageContent, PageCacheParameters pa } + @Override + public void addBytes(final IHTMLPage page, final byte[] content, final PageCacheParameters pageCacheParams) { + if (UtilMethods.isEmpty(() -> page.getIdentifier()) || pageCacheParams == null || content == null) { + return; + } + + final String cacheKey = pageCacheParams.getKey(); + final long ttl = page.getCacheTTL() > 0 ? page.getCacheTTL() * 1000L : 60L * 60 * 24 * 7 * 1000; // 1 week + + Logger.info(this.getClass(), () -> "PageCache Put (bytes): ttl:" + ttl + " key:" + cacheKey + " size:" + content.length); + + // Store bytes directly - avoids String conversion overhead + this.cache.put(cacheKey, new CacheValueImpl(content, ttl), PRIMARY_GROUP); + } + @Override + @Deprecated public String get(final IHTMLPage page, final PageCacheParameters pageCacheParams) { if (UtilMethods.isEmpty(() -> page.getIdentifier()) || pageCacheParams == null) { @@ -96,14 +114,55 @@ public String get(final IHTMLPage page, final PageCacheParameters pageCacheParam if (cachedValue instanceof String) { return (String) cachedValue; } + if (cachedValue instanceof byte[]) { + // Convert bytes to String for backward compatibility + return new String((byte[]) cachedValue, StandardCharsets.UTF_8); + } if (cachedValue instanceof CacheValue) { - return (String) ((CacheValue) cachedValue).getValue(); + final Object value = ((CacheValue) cachedValue).getValue(); + if (value instanceof String) { + return (String) value; + } + if (value instanceof byte[]) { + return new String((byte[]) value, StandardCharsets.UTF_8); + } } return null; } + @Override + public byte[] getBytes(final IHTMLPage page, final PageCacheParameters pageCacheParams) { + if (UtilMethods.isEmpty(() -> page.getIdentifier()) || pageCacheParams == null) { + return null; + } + + final String cacheKey = pageCacheParams.getKey(); + + // Look up the cached page + final Object cachedValue = this.cache.getNoThrow(cacheKey, PRIMARY_GROUP); + + if (cachedValue instanceof byte[]) { + return (byte[]) cachedValue; + } + if (cachedValue instanceof String) { + // Convert String to bytes for backward compatibility with old cache entries + return ((String) cachedValue).getBytes(StandardCharsets.UTF_8); + } + if (cachedValue instanceof CacheValue) { + final Object value = ((CacheValue) cachedValue).getValue(); + if (value instanceof byte[]) { + return (byte[]) value; + } + if (value instanceof String) { + return ((String) value).getBytes(StandardCharsets.UTF_8); + } + } + + return null; + } + @Override public void remove(IHTMLPage page) { diff --git a/dotCMS/src/main/java/com/dotmarketing/business/web/LanguageWebAPIImpl.java b/dotCMS/src/main/java/com/dotmarketing/business/web/LanguageWebAPIImpl.java index 884b93aa6aff..f5dda15f6a63 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/web/LanguageWebAPIImpl.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/web/LanguageWebAPIImpl.java @@ -12,13 +12,11 @@ import com.dotmarketing.util.PageMode; import com.dotmarketing.util.UtilMethods; import com.dotmarketing.util.WebKeys; -import com.liferay.portal.language.LanguageUtil; import com.liferay.portal.struts.MultiMessageResources; import com.liferay.portal.struts.MultiMessageResourcesFactory; - +import java.util.Locale; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpSession; -import java.util.Locale; /** * Implement LanguageWebAPI methods to manage language cache and language struts @@ -30,7 +28,7 @@ */ public class LanguageWebAPIImpl implements LanguageWebAPI { - private LanguageAPI langAPI; + private final LanguageAPI langAPI; public LanguageWebAPIImpl() { langAPI = APILocator.getLanguageAPI(); @@ -54,30 +52,24 @@ public void clearCache() throws DotRuntimeException { // only try internal session and attributes private Language currentLanguage(HttpServletRequest httpRequest) { HttpSession sessionOpt = httpRequest.getSession(false); - Language lang = null; try{ if(sessionOpt !=null){ if(sessionOpt.getAttribute("tm_lang")!=null){ - lang = langAPI.getLanguage((String) sessionOpt.getAttribute("tm_lang")); + return langAPI.getLanguage((String) sessionOpt.getAttribute("tm_lang")); }else{ - lang= langAPI.getLanguage((String) sessionOpt.getAttribute(com.dotmarketing.util.WebKeys.HTMLPAGE_LANGUAGE)); + return langAPI.getLanguage((String) sessionOpt.getAttribute(com.dotmarketing.util.WebKeys.HTMLPAGE_LANGUAGE)); } } else if(UtilMethods.isSet(httpRequest.getAttribute(HTMLPAGE_CURRENT_LANGUAGE))){ - lang= langAPI.getLanguage((String) httpRequest.getAttribute(HTMLPAGE_CURRENT_LANGUAGE)); - } - if(lang==null) { - lang =langAPI.getDefaultLanguage(); + return langAPI.getLanguage((String) httpRequest.getAttribute(HTMLPAGE_CURRENT_LANGUAGE)); } } catch(Exception e){ - lang =langAPI.getDefaultLanguage(); + // no log } - - - return lang; + return langAPI.getDefaultLanguage(); } @@ -123,22 +115,19 @@ private Language futureLanguage(HttpServletRequest httpRequest,Language currentL public Language getLanguage(HttpServletRequest httpRequest) { final Language current = currentLanguage(httpRequest); final Language future = futureLanguage(httpRequest,current); - final Locale locale = null != future.getCountryCode()? new Locale(future.getLanguageCode(), future.getCountryCode()): new Locale(future.getLanguageCode()); - HttpSession sessionOpt = httpRequest.getSession(false); + final Locale locale = null != future.getCountryCode() + ? new Locale(future.getLanguageCode(), future.getCountryCode()) + : new Locale(future.getLanguageCode()); httpRequest.setAttribute(HTMLPAGE_CURRENT_LANGUAGE, String.valueOf(future.getId())); - httpRequest.setAttribute(WebKeys.LOCALE, locale); // if someone is changing langauges, we need a session - if(!current.equals(future)){ - sessionOpt = httpRequest.getSession(true); - } - if(sessionOpt!=null){ + if (current.getId() != future.getId()) { + HttpSession sessionOpt = httpRequest.getSession(); //only set in session if we are not in a timemachine if(sessionOpt.getAttribute("tm_lang")==null){ sessionOpt.setAttribute(WebKeys.HTMLPAGE_LANGUAGE, String.valueOf(future.getId())); boolean ADMIN_MODE = PageMode.get(httpRequest).isAdmin; - if (ADMIN_MODE == false || httpRequest.getParameter("leftMenu") == null) { sessionOpt.setAttribute(WebKeys.Globals_FRONTEND_LOCALE_KEY, locale); httpRequest.setAttribute(WebKeys.Globals_FRONTEND_LOCALE_KEY, locale); @@ -206,4 +195,4 @@ private Locale getGlocalLocale(final HttpServletRequest request){ } } -} \ No newline at end of file +} diff --git a/dotCMS/src/main/java/com/dotmarketing/filters/CMSFilter.java b/dotCMS/src/main/java/com/dotmarketing/filters/CMSFilter.java index 6f6f68606873..44d449a60302 100644 --- a/dotCMS/src/main/java/com/dotmarketing/filters/CMSFilter.java +++ b/dotCMS/src/main/java/com/dotmarketing/filters/CMSFilter.java @@ -3,7 +3,6 @@ import com.dotcms.api.web.HttpServletRequestThreadLocal; import com.dotcms.api.web.HttpServletResponseThreadLocal; import com.dotcms.exception.ExceptionUtil; -import com.dotcms.rest.config.DotServiceLocatorImpl.QuietServiceShutdownException; import com.dotcms.vanityurl.filters.VanityUrlRequestWrapper; import com.dotcms.visitor.business.VisitorAPI; import com.dotcms.visitor.domain.Visitor; @@ -14,22 +13,29 @@ import com.dotmarketing.db.DbConnectionFactory; import com.dotmarketing.portlets.rules.business.RulesEngine; import com.dotmarketing.portlets.rules.model.Rule; -import com.dotmarketing.util.*; +import com.dotmarketing.util.Config; +import com.dotmarketing.util.Logger; +import com.dotmarketing.util.NumberOfTimeVisitedCounter; +import com.dotmarketing.util.PageMode; +import com.dotmarketing.util.UtilMethods; +import com.dotmarketing.util.WebKeys; import io.vavr.Tuple2; import io.vavr.control.Try; - -import java.util.Set; -import javax.servlet.*; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.io.StringWriter; import java.util.Optional; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; public class CMSFilter implements Filter { - private final HttpServletRequestThreadLocal requestThreadLocal = HttpServletRequestThreadLocal.INSTANCE; - private final HttpServletResponseThreadLocal responseThreadLocal = HttpServletResponseThreadLocal.INSTANCE; + private CMSUrlUtil urlUtil = CMSUrlUtil.getInstance(); private static VisitorAPI visitorAPI = APILocator.getVisitorAPI(); private final String RELATIVE_ASSET_PATH = APILocator.getFileAssetAPI().getRelativeAssetsRootPath(); @@ -74,8 +80,6 @@ public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) } finally { DbConnectionFactory.closeSilently(); } - - } private void doFilterInternal(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException { @@ -86,8 +90,8 @@ private void doFilterInternal(ServletRequest req, ServletResponse res, FilterCha final long languageId = WebAPILocator.getLanguageWebAPI().getLanguage(request).getId(); // Set the request/response in the thread local. - this.requestThreadLocal.setRequest(request); - this.responseThreadLocal.setResponse(response); + HttpServletRequestThreadLocal.INSTANCE.setRequest(request); + HttpServletResponseThreadLocal.INSTANCE.setResponse(response); // run rules engine for all requests RulesEngine.fireRules(request, response, Rule.FireOn.EVERY_REQUEST); diff --git a/dotCMS/src/main/java/com/dotmarketing/filters/CMSUrlUtil.java b/dotCMS/src/main/java/com/dotmarketing/filters/CMSUrlUtil.java index efa023d59dad..a2de79a4627a 100644 --- a/dotCMS/src/main/java/com/dotmarketing/filters/CMSUrlUtil.java +++ b/dotCMS/src/main/java/com/dotmarketing/filters/CMSUrlUtil.java @@ -1,5 +1,14 @@ package com.dotmarketing.filters; +import static com.dotmarketing.business.PermissionAPI.PERMISSION_READ; +import static com.dotmarketing.filters.CMSFilter.CMS_INDEX_PAGE; +import static com.dotmarketing.filters.Constants.CMS_FILTER_QUERY_STRING_OVERRIDE; +import static com.dotmarketing.filters.Constants.CMS_FILTER_URI_OVERRIDE; +import static com.liferay.util.StringPool.FORWARD_SLASH; +import static com.liferay.util.StringPool.PERIOD; +import static com.liferay.util.StringPool.UNDERLINE; +import static java.util.stream.Collectors.toSet; + import com.dotcms.contenttype.model.type.BaseContentType; import com.dotmarketing.beans.Host; import com.dotmarketing.beans.Identifier; @@ -27,10 +36,6 @@ import com.liferay.util.Xss; import io.vavr.Tuple; import io.vavr.Tuple2; - -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; import java.io.IOException; import java.io.UnsupportedEncodingException; import java.net.URI; @@ -43,15 +48,9 @@ import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; - -import static com.dotmarketing.business.PermissionAPI.PERMISSION_READ; -import static com.dotmarketing.filters.CMSFilter.CMS_INDEX_PAGE; -import static com.dotmarketing.filters.Constants.CMS_FILTER_QUERY_STRING_OVERRIDE; -import static com.dotmarketing.filters.Constants.CMS_FILTER_URI_OVERRIDE; -import static com.liferay.util.StringPool.FORWARD_SLASH; -import static com.liferay.util.StringPool.PERIOD; -import static com.liferay.util.StringPool.UNDERLINE; -import static java.util.stream.Collectors.toSet; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; /** * Utilitary class used by the CMS Filter @@ -556,8 +555,10 @@ public boolean isUnauthorizedAndHandleError(final Permissionable permissionable, "CHECKING PERMISSION: Page doesn't have anonymous access [" + requestedURIForLogging + "]"); Logger.debug(this.getClass(), "401 URI = " + requestedURIForLogging); Logger.debug(this.getClass(), "Unauthorized URI = " + requestedURIForLogging); - - request.getSession().setAttribute(com.dotmarketing.util.WebKeys.REDIRECT_AFTER_LOGIN, requestedURIForLogging); + if (request.getSession(false) != null) { + request.getSession() + .setAttribute(com.dotmarketing.util.WebKeys.REDIRECT_AFTER_LOGIN, requestedURIForLogging); + } response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "The requested page/file is unauthorized"); return true; } else if (!permissionAPI.getRolesWithPermission(permissionable, PERMISSION_READ) diff --git a/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/transform/strategy/FileViewStrategy.java b/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/transform/strategy/FileViewStrategy.java index ac46be743e8b..6099307a3171 100644 --- a/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/transform/strategy/FileViewStrategy.java +++ b/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/transform/strategy/FileViewStrategy.java @@ -102,7 +102,7 @@ public Map transform(final Field field, final Contentlet content if(fileAsContentOptional.isEmpty()) { //Prevent NPE - Logger.warn(this, "Live FileAsset not found for identifier: " + fileAssetIdentifier); + Logger.debug(this, "Live FileAsset not found for identifier: " + fileAssetIdentifier); return Map.of(); } diff --git a/dotCMS/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java b/dotCMS/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java index a4ccec5a884e..b72aa7408b2e 100644 --- a/dotCMS/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java +++ b/dotCMS/src/main/java/org/apache/velocity/runtime/parser/node/ASTMethod.java @@ -140,30 +140,29 @@ public Object execute(Object o, InternalContextAdapter context) * at execution time. There can be no in-node caching, * but if we are careful, we can do it in the context. */ - Object [] params = new Object[paramCount]; + Object[] params = new Object[paramCount]; - /* - * sadly, we do need recalc the values of the args, as this can - * change from visit to visit - */ - final Class[] paramClasses = - paramCount > 0 ? new Class[paramCount] : ArrayUtils.EMPTY_CLASS_ARRAY; + /* + * sadly, we do need recalc the values of the args, as this can + * change from visit to visit + */ + final Class[] paramClasses = + paramCount > 0 ? new Class[paramCount] : ArrayUtils.EMPTY_CLASS_ARRAY; - for (int j = 0; j < paramCount; j++) - { + for (int j = 0; j < paramCount; j++) { params[j] = jjtGetChild(j + 1).value(context); - if (params[j] != null) - { + if (params[j] != null) { paramClasses[j] = params[j].getClass(); } } - - VelMethod method = ClassUtils.getMethod(methodName, params, paramClasses, - o, context, this, strictRef); - if (method == null) return null; - try - { + VelMethod method = ClassUtils.getMethod(methodName, params, paramClasses, + o, context, this, strictRef); + if (method == null) { + return null; + } + + try { /* * get the returned object. It may be null, and that is * valid for something declared with a void return type. @@ -189,13 +188,13 @@ public Object execute(Object o, InternalContextAdapter context) { return handleInvocationException(o, context, ite.getTargetException()); } - + /** Can also be thrown by method invocation **/ catch( IllegalArgumentException t ) { return handleInvocationException(o, context, t); } - + /** * pass through application level runtime exceptions */ diff --git a/dotCMS/src/test/java/com/dotcms/concurrent/DotConcurrentFactoryTest.java b/dotCMS/src/test/java/com/dotcms/concurrent/DotConcurrentFactoryTest.java index 7365ab96b8b3..0d5070950589 100644 --- a/dotCMS/src/test/java/com/dotcms/concurrent/DotConcurrentFactoryTest.java +++ b/dotCMS/src/test/java/com/dotcms/concurrent/DotConcurrentFactoryTest.java @@ -1,14 +1,17 @@ package com.dotcms.concurrent; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; + import com.dotcms.UnitTestBase; import com.dotcms.concurrent.DotConcurrentFactory.SubmitterConfigBuilder; import com.dotmarketing.util.DateUtil; import com.dotmarketing.util.json.JSONException; -import org.junit.Test; - import java.util.ArrayList; import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -17,19 +20,14 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; import java.util.stream.IntStream; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; +import org.junit.Test; public class DotConcurrentFactoryTest extends UnitTestBase { /** * Method to test: {@link ConditionalSubmitter#submit(Supplier, Supplier)} - * Given Scenario: run 100 task but only 6 should be submitted as a scenario 1, the rest should be scenario 2 - * ExpectedResult: 6 ran on scenario 1 and 94 on scenario 2 + * Given Scenario: run tasks but only 6 should be submitted as scenario 1, the rest should be scenario 2 + * ExpectedResult: 6 ran on scenario 1 and the rest on scenario 2 * */ @Test @@ -38,26 +36,42 @@ public void test_Conditional_Submitter() throws ExecutionException, Interrupted final String scenario1String = "scenario1"; final String scenario2String = "scenario2"; final int slots = 6; - ExecutorService executorService = Executors.newFixedThreadPool(10); - final ConditionalSubmitter conditionalExecutor = DotConcurrentFactory.getInstance().createConditionalSubmitter(slots); + final int taskCount = 12; + ExecutorService executorService = Executors.newFixedThreadPool(taskCount); + // Use 0 second timeout so tasks fall back immediately when no slot is available + // (default 1s timeout would cause tasks to wait and acquire released slots) + final ConditionalSubmitter conditionalExecutor = DotConcurrentFactory.getInstance() + .createConditionalSubmitter(slots, 0); final List> futures = new ArrayList<>(); - int scenarios1Count = 0; - int scenarios2Count = 0; - final Supplier supplier1 = ()-> { - System.out.println("Supplier1, a " + System.currentTimeMillis()); - DateUtil.sleep(DateUtil.EIGHT_SECOND_MILLIS); - System.out.println("Supplier1, b " + System.currentTimeMillis()); + // Use latches to ensure all tasks start simultaneously and compete for slots + final CountDownLatch readyLatch = new CountDownLatch(taskCount); + final CountDownLatch startLatch = new CountDownLatch(1); + + final Supplier supplier1 = ()-> { + DateUtil.sleep(200); return scenario1String; }; final Supplier supplier2 = ()-> scenario2String; - for (int i = 0; i < 20; ++i) { - futures.add(executorService.submit(()-> conditionalExecutor.submit(supplier1, supplier2))); + for (int i = 0; i < taskCount; ++i) { + futures.add(executorService.submit(() -> { + readyLatch.countDown(); + try { + startLatch.await(); // Wait for all threads to be ready + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + return conditionalExecutor.submit(supplier1, supplier2); + })); } - for (final Future future: futures) { + readyLatch.await(); // Wait for all threads to be ready + startLatch.countDown(); // Release all threads simultaneously + int scenarios1Count = 0; + int scenarios2Count = 0; + for (final Future future: futures) { final String result = future.get(); if (scenario2String.equals(result)) { scenarios2Count++; @@ -67,17 +81,30 @@ public void test_Conditional_Submitter() throws ExecutionException, Interrupted } assertEquals(6, scenarios1Count); - assertEquals(14, scenarios2Count); + assertEquals(6, scenarios2Count); + + // Second round to verify slot release works correctly + final CountDownLatch readyLatch2 = new CountDownLatch(taskCount); + final CountDownLatch startLatch2 = new CountDownLatch(1); scenarios2Count = scenarios1Count = 0; futures.clear(); - for (int i = 0; i < 20; ++i) { - - futures.add(executorService.submit(()-> conditionalExecutor.submit(supplier1, supplier2))); + for (int i = 0; i < taskCount; ++i) { + futures.add(executorService.submit(() -> { + readyLatch2.countDown(); + try { + startLatch2.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + return conditionalExecutor.submit(supplier1, supplier2); + })); } - for (final Future future: futures) { + readyLatch2.await(); + startLatch2.countDown(); + for (final Future future: futures) { final String result = future.get(); if (scenario2String.equals(result)) { scenarios2Count++; @@ -87,8 +114,9 @@ public void test_Conditional_Submitter() throws ExecutionException, Interrupted } assertEquals(6, scenarios1Count); - assertEquals(14, scenarios2Count); + assertEquals(6, scenarios2Count); + executorService.shutdown(); } /** @@ -106,46 +134,36 @@ public void testDefaultOne_Single_Submitter_Config() throws JSONException, Execu final DotSubmitter submitter = dotConcurrentFactory.getSingleSubmitter(submitterName); - final List futures = new ArrayList<>(); - System.out.println(submitter); + final List> futures = new ArrayList<>(); - IntStream.range(0, 10).forEach( - n -> { - futures.add(submitter.submit(new PrintTask("Thread" + n))); - } + // Reduced from 10 to 5 iterations - still verifies single-threaded execution + IntStream.range(0, 5).forEach( + n -> futures.add(submitter.submit(new PrintTask("Thread" + n))) ); - //check active thread, if zero then shut down the thread pool - for (final Future future : futures) { - + // Wait for all tasks to complete + for (final Future future : futures) { future.get(); } - System.out.print("Staring a new one submitter"); - final DotSubmitter submitter2 = dotConcurrentFactory.getSingleSubmitter(submitterName); - System.out.println(submitter2); + assertSame(submitter, submitter2); - assertTrue(submitter == submitter2); - - final List futures2 = new ArrayList<>(); - IntStream.range(0, 10).forEach( - n -> { - futures2.add(submitter2.submit(new PrintTask("Thread" + n))); - } + final List> futures2 = new ArrayList<>(); + IntStream.range(0, 5).forEach( + n -> futures2.add(submitter2.submit(new PrintTask("Thread" + n))) ); - //check active thread, if zero then shut down the thread pool - for (final Future future : futures2) { - + // Wait for all tasks to complete + for (final Future future : futures2) { future.get(); } } @Test - public void testDefaultOne_Submitter_Config() throws JSONException{ + public void testDefaultOne_Submitter_Config() throws JSONException, InterruptedException, ExecutionException { final String submitterName = "testsubmitter"; final DotConcurrentFactory dotConcurrentFactory = @@ -157,70 +175,40 @@ public void testDefaultOne_Submitter_Config() throws JSONException{ .maxPoolSize(4).queueCapacity(500).build() ); - System.out.println(submitter); - - IntStream.range(0, 40).forEach( - n -> { - - if (n % 10 == 0) { - - System.out.println(submitter); - } - submitter.execute(new PrintTask("Thread" + n)); - } + // Reduced from 40 to 10 iterations - still tests thread pool behavior + final List> futures = new ArrayList<>(); + IntStream.range(0, 10).forEach( + n -> futures.add(submitter.submit(new PrintTask("Thread" + n))) ); - //check active thread, if zero then shut down the thread pool - for (;;) { - int count = submitter.getActiveCount(); - System.out.println("Active Threads : " + count); - try { - Thread.sleep(100); - } catch (InterruptedException e) { - e.printStackTrace(); - } - if (count == 0) { - //submitter.shutdown(); - break; - } + // Wait for completion instead of polling + for (final Future future : futures) { + future.get(); } - System.out.print("Staring a new one submitter"); - final DotSubmitter submitter2 = dotConcurrentFactory.getSubmitter(submitterName); - System.out.println(submitter2); + assertSame(submitter, submitter2); - assertTrue(submitter == submitter2); - - IntStream.range(0, 20).forEach( - n -> { - submitter2.execute(new PrintTask("Thread" + n)); - } + // Reduced from 20 to 5 iterations + final List> futures2 = new ArrayList<>(); + IntStream.range(0, 5).forEach( + n -> futures2.add(submitter2.submit(new PrintTask("Thread" + n))) ); - //check active thread, if zero then shut down the thread pool - for (;;) { - int count = submitter2.getActiveCount(); - System.out.println("Active Threads : " + count); - try { - Thread.sleep(100); - } catch (InterruptedException e) { - e.printStackTrace(); - } - if (count == 0) { - submitter2.shutdown(); - break; - } + for (final Future future : futures2) { + future.get(); } + submitter2.shutdown(); } @Test - public void testDefaultOne_Submitter_Config_shutdown_keep_config() throws JSONException{ + public void testDefaultOne_Submitter_Config_shutdown_keep_config() + throws JSONException, InterruptedException, ExecutionException { - final String submitterName = "testsubmitter"; + final String submitterName = "testsubmitter_shutdown"; final DotConcurrentFactory dotConcurrentFactory = DotConcurrentFactory.getInstance(); @@ -230,76 +218,46 @@ public void testDefaultOne_Submitter_Config_shutdown_keep_config() throws JSONEx .maxPoolSize(4).queueCapacity(500).build() ); - System.out.println(submitter); - - IntStream.range(0, 40).forEach( - n -> { - - if (n % 10 == 0) { - - System.out.println(submitter); - } - submitter.execute(new PrintTask("Thread" + n)); - } + // Reduced from 40 to 8 iterations - still tests config preservation + final List> futures = new ArrayList<>(); + IntStream.range(0, 8).forEach( + n -> futures.add(submitter.submit(new PrintTask("Thread" + n))) ); assertEquals(2, submitter.getPoolSize()); assertEquals(4, submitter.getMaxPoolSize()); - //check active thread, if zero then shut down the thread pool - for (;;) { - int count = submitter.getActiveCount(); - System.out.println("Active Threads : " + count); - try { - Thread.sleep(100); - } catch (InterruptedException e) { - e.printStackTrace(); - } - if (count == 0) { - //submitter.shutdown(); - break; - } + // Wait for completion instead of polling + for (final Future future : futures) { + future.get(); } submitter.shutdown(); - System.out.print("Staring a new one submitter"); - + // After shutdown, getting same name should create new submitter with same config final DotSubmitter submitter2 = dotConcurrentFactory.getSubmitter(submitterName); - System.out.println(submitter2); + assertNotSame(submitter, submitter2); - assertFalse(submitter == submitter2); - - IntStream.range(0, 20).forEach( - n -> { - submitter2.execute(new PrintTask("Thread" + n)); - } + // Reduced from 20 to 5 iterations + final List> futures2 = new ArrayList<>(); + IntStream.range(0, 5).forEach( + n -> futures2.add(submitter2.submit(new PrintTask("Thread" + n))) ); assertEquals(2, submitter2.getPoolSize()); assertEquals(4, submitter2.getMaxPoolSize()); - //check active thread, if zero then shut down the thread pool - for (;;) { - int count = submitter2.getActiveCount(); - System.out.println("Active Threads : " + count); - try { - Thread.sleep(100); - } catch (InterruptedException e) { - e.printStackTrace(); - } - if (count == 0) { - submitter2.shutdown(); - break; - } + for (final Future future : futures2) { + future.get(); } + submitter2.shutdown(); } @Test - public void testDefaultOne() throws JSONException{ + public void testDefaultOne() throws JSONException, InterruptedException, ExecutionException { final DotConcurrentFactory dotConcurrentFactory = DotConcurrentFactory.getInstance(); @@ -307,68 +265,37 @@ public void testDefaultOne() throws JSONException{ final DotSubmitter submitter = dotConcurrentFactory.getSubmitter(); - System.out.println(submitter); - - IntStream.range(0, 40).forEach( - n -> { - - if (n % 10 == 0) { - - System.out.println(submitter); - } - submitter.execute(new PrintTask("Thread" + n)); - } + // Reduced from 40 to 10 iterations + final List> futures = new ArrayList<>(); + IntStream.range(0, 10).forEach( + n -> futures.add(submitter.submit(new PrintTask("Thread" + n))) ); - //check active thread, if zero then shut down the thread pool - for (;;) { - int count = submitter.getActiveCount(); - System.out.println("Active Threads : " + count); - try { - Thread.sleep(100); - } catch (InterruptedException e) { - e.printStackTrace(); - } - if (count == 0) { - //submitter.shutdown(); - break; - } + // Wait for completion instead of polling + for (final Future future : futures) { + future.get(); } - System.out.print("Staring a new one submitter"); - final DotSubmitter submitter2 = dotConcurrentFactory.getSubmitter(); - System.out.println(submitter2); + assertSame(submitter, submitter2); - assertTrue(submitter == submitter2); - - IntStream.range(0, 20).forEach( - n -> { - submitter2.execute(new PrintTask("Thread" + n)); - } + // Reduced from 20 to 5 iterations + final List> futures2 = new ArrayList<>(); + IntStream.range(0, 5).forEach( + n -> futures2.add(submitter2.submit(new PrintTask("Thread" + n))) ); - //check active thread, if zero then shut down the thread pool - for (;;) { - int count = submitter2.getActiveCount(); - System.out.println("Active Threads : " + count); - try { - Thread.sleep(100); - } catch (InterruptedException e) { - e.printStackTrace(); - } - if (count == 0) { - submitter2.shutdown(); - break; - } + for (final Future future : futures2) { + future.get(); } + submitter2.shutdown(); } - class PrintTask implements Runnable { + static class PrintTask implements Runnable { String name; @@ -378,16 +305,12 @@ public PrintTask(String name){ @Override public void run() { - - System.out.println(name + " is running"); - + // Reduced from 500ms to 20ms - just needs to simulate some work try { - Thread.sleep(500); + Thread.sleep(20); } catch (InterruptedException e) { - e.printStackTrace(); + Thread.currentThread().interrupt(); } - - System.out.println(name + " is running"); } } @@ -397,54 +320,55 @@ public void run() { /** * This tests that the delayed queue will run jobs in the future, efficiently - * - * @throws Exception + * + * @throws Exception if test fails */ @Test public void test_delayed_queue() throws Exception{ - - // will kill it from the cache in some time. + final DotSubmitter submitter = DotConcurrentFactory.getInstance().getSubmitter(); - final AtomicInteger aInt=new AtomicInteger(0); - - // add 50 to the atomicInteger, 3 seconds in the future - int runs = 50; - for(int i=0;i aInt.addAndGet(1), 1, TimeUnit.SECONDS); + final AtomicInteger aInt = new AtomicInteger(0); + + // Reduced from 50 to 10 iterations - still tests delayed execution + final int runs = 10; + for (int i = 0; i < runs; i++) { + // Reduced delay from 1 second to 100ms + submitter.delay(() -> aInt.addAndGet(1), 100, TimeUnit.MILLISECONDS); } - - // None of the jobs have been run - assert(aInt.get()==0); - - // rest. I must rest - Thread.sleep(2000); - - // all of the jobs have been run - assert(aInt.get()==50); + + // None of the jobs have been run yet + assertEquals(0, aInt.get()); + + // Wait for delayed tasks to complete (200ms buffer) + Thread.sleep(300); + + // All of the jobs should have been run + assertEquals(runs, aInt.get()); } /** - * Test the CompleatableFuture any; in this case the last one should be the fastest + * Test the CompletableFuture any; in this case the last one should be the fastest * - * @throws Exception + * @throws Exception if test fails */ @Test public void test_toCompletableAnyFuture_success() throws Exception{ - // will kill it from the cache in some time. final DotSubmitter submitter = DotConcurrentFactory.getInstance().getSubmitter(); final List> futures = new ArrayList<>(); - for (int i = 1; i <= 10; ++i) { - final int finalIndex = i; - futures.add(submitter.submit(() ->{ - DateUtil.sleep(Math.abs(finalIndex - 10) * 1000); + // Reduced from 10 iterations with up to 9 second delays to 5 iterations with 100ms delays + for (int i = 1; i <= 5; ++i) { + final int finalIndex = i; + futures.add(submitter.submit(() -> { + // Last one (index 5) has 0 delay, first one (index 1) has 400ms delay + DateUtil.sleep(Math.abs(finalIndex - 5) * 100); return finalIndex; })); } final CompletableFuture completableFuture = DotConcurrentFactory.getInstance().toCompletableAnyFuture(futures); final Integer result = completableFuture.get(); - assertEquals("The last one should be the fastest", 10, result.intValue()); + assertEquals("The last one should be the fastest", 5, result.intValue()); } -} \ No newline at end of file +}