Merge "J-527: All Statistics objects are verified to be immutable. If required, builders are thread-safe. Deprecated snapshot() methods. Fixed SlidingWindowTimeReservoir which was unable to calculate metrics correctly for updates with time older than the time of the sliding window creation (as a result, after the application startup, first requests weren't added to the calculation due to lazy creation of SlidingWindowTimeReservoir). Extended tests."

Author: pavelbucek

core-server/src/main/java/org/glassfish/jersey/server/internal/monitoring/ExceptionMapperStatisticsImpl.java

@@ -41,6 +41,7 @@
 package org.glassfish.jersey.server.internal.monitoring;
 
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.Map;
 
 import org.glassfish.jersey.server.monitoring.ExceptionMapperStatistics;
@@ -56,10 +57,12 @@ final class ExceptionMapperStatisticsImpl implements ExceptionMapperStatistics {
 
     /**
      * Builder of exception mapper statistics.
+     * <p/>
+     * This builder does not need to be threadsafe since it's called only from the jersey-background-task-scheduler.
      */
     static class Builder {
 
-        private Map<Class<?>, Long> exceptionMapperExecutionCount = Maps.newHashMap();
+        private Map<Class<?>, Long> exceptionMapperExecutionCountMap = Maps.newHashMap();
         private long successfulMappings;
         private long unsuccessfulMappings;
         private long totalMappings;
@@ -92,9 +95,9 @@ void addMapping(final boolean success, final int count) {
         void addExceptionMapperExecution(final Class<?> mapper, final int count) {
             cached = null;
 
-            Long cnt = exceptionMapperExecutionCount.get(mapper);
+            Long cnt = exceptionMapperExecutionCountMap.get(mapper);
             cnt = cnt == null ? count : cnt + count;
-            exceptionMapperExecutionCount.put(mapper, cnt);
+            exceptionMapperExecutionCountMap.put(mapper, cnt);
         }
 
         /**
@@ -104,7 +107,7 @@ void addExceptionMapperExecution(final Class<?> mapper, final int count) {
          */
         public ExceptionMapperStatisticsImpl build() {
             if (cached == null) {
-                cached = new ExceptionMapperStatisticsImpl(Collections.unmodifiableMap(exceptionMapperExecutionCount),
+                cached = new ExceptionMapperStatisticsImpl(new HashMap<>(this.exceptionMapperExecutionCountMap),
                         successfulMappings, unsuccessfulMappings, totalMappings);
             }
 
@@ -119,7 +122,7 @@ public ExceptionMapperStatisticsImpl build() {
 
     private ExceptionMapperStatisticsImpl(final Map<Class<?>, Long> exceptionMapperExecutionCount, final long successfulMappings,
                                           final long unsuccessfulMappings, final long totalMappings) {
-        this.exceptionMapperExecutionCount = exceptionMapperExecutionCount;
+        this.exceptionMapperExecutionCount = Collections.unmodifiableMap(exceptionMapperExecutionCount);
         this.successfulMappings = successfulMappings;
         this.unsuccessfulMappings = unsuccessfulMappings;
         this.totalMappings = totalMappings;

core-server/src/main/java/org/glassfish/jersey/server/internal/monitoring/ExecutionStatisticsImpl.java

@@ -52,44 +52,51 @@
 import jersey.repackaged.com.google.common.collect.Maps;
 
 /**
- * Execution statistics.
+ * Immutable Execution statistics.
  *
  * @author Miroslav Fuksa
+ * @author Stepan Vavra (stepan.vavra at oracle.com)
  */
 final class ExecutionStatisticsImpl implements ExecutionStatistics {
 
     /**
      * Builder of execution statistics.
+     * <p/>
+     * Must be thread-safe.
      */
     static class Builder {
 
-        private long lastStartTime;
+        private volatile long lastStartTime;
         private final Map<Long, TimeWindowStatisticsImpl.Builder> intervalStatistics;
 
         /**
          * Create a new builder.
          */
         public Builder() {
-            this.intervalStatistics = new HashMap<>(6);
-            addInterval(0, TimeUnit.MILLISECONDS);
-            addInterval(1, TimeUnit.SECONDS);
-            addInterval(15, TimeUnit.SECONDS);
-            addInterval(1, TimeUnit.MINUTES);
-            addInterval(15, TimeUnit.MINUTES);
-            addInterval(1, TimeUnit.HOURS);
+            // create unmodifiable map to ensure that an iteration in the build() won't have multi-threading issues
+            this.intervalStatistics = Collections.unmodifiableMap(new HashMap<Long, TimeWindowStatisticsImpl.Builder>(6) {
+                {
+                    addInterval(0, TimeUnit.MILLISECONDS, this);
+                    addInterval(1, TimeUnit.SECONDS, this);
+                    addInterval(15, TimeUnit.SECONDS, this);
+                    addInterval(1, TimeUnit.MINUTES, this);
+                    addInterval(15, TimeUnit.MINUTES, this);
+                    addInterval(1, TimeUnit.HOURS, this);
+                }
+            });
         }
 
-        private void addInterval(final long interval, final TimeUnit timeUnit) {
+        private void addInterval(final long interval, final TimeUnit timeUnit, final Map map) {
             final long intervalInMillis = timeUnit.toMillis(interval);
-            intervalStatistics.put(intervalInMillis, new TimeWindowStatisticsImpl.Builder(intervalInMillis,
+            map.put(intervalInMillis, new TimeWindowStatisticsImpl.Builder(intervalInMillis,
                     TimeUnit.MILLISECONDS));
         }
 
         /**
          * Add execution of a target.
          *
          * @param startTime (Unix timestamp format)
-         * @param duration Duration of target execution in milliseconds.
+         * @param duration  Duration of target execution in milliseconds.
          */
         void addExecution(final long startTime, final long duration) {
             for (final TimeWindowStatisticsImpl.Builder statBuilder : intervalStatistics.values()) {
@@ -112,18 +119,18 @@ public ExecutionStatisticsImpl build() {
 
             // cache when request rate is 0
 
-            return new ExecutionStatisticsImpl(lastStartTime, Collections.unmodifiableMap(newIntervalStatistics));
+            return new ExecutionStatisticsImpl(lastStartTime, newIntervalStatistics);
         }
     }
 
     static final ExecutionStatistics EMPTY = new Builder().build();
 
-    private final Date lastStartTime;
+    private final long lastStartTime;
     private final Map<Long, TimeWindowStatistics> timeWindowStatistics;
 
     @Override
     public Date getLastStartTime() {
-        return lastStartTime;
+        return new Date(lastStartTime);
     }
 
     @Override
@@ -133,13 +140,13 @@ public Map<Long, TimeWindowStatistics> getTimeWindowStatistics() {
 
     @Override
     public ExecutionStatistics snapshot() {
-        // snapshot functionality not yet implemented
+        // this object is immutable (TimeWindowStatistics are immutable as well)
         return this;
     }
 
     private ExecutionStatisticsImpl(final long lastStartTime, final Map<Long, TimeWindowStatistics> timeWindowStatistics) {
-        this.lastStartTime = new Date(lastStartTime);
-        this.timeWindowStatistics = timeWindowStatistics;
+        this.lastStartTime = lastStartTime;
+        this.timeWindowStatistics = Collections.unmodifiableMap(timeWindowStatistics);
     }
 
 }

core-server/src/main/java/org/glassfish/jersey/server/internal/monitoring/MonitoringStatisticsImpl.java

@@ -59,13 +59,33 @@
 
 /**
  * Monitoring statistics implementation.
+ * <p/>
+ * This object is loosely immutable (i.e., {@link #getResourceClassStatistics()} and {@link #getUriStatistics()} gets updated on
+ * access). As a result, it is unnecessary to call {@link #snapshot()}.
  *
  * @author Miroslav Fuksa
  */
 final class MonitoringStatisticsImpl implements MonitoringStatistics {
 
     /**
      * Builder of monitoring statistics.
+     * <p/>
+     * This builder does not need to be threadsafe as it's only accessed by jersey-background-task-scheduler. However, {@link
+     * #BUILDING_FUNCTION} is triggered when it is accessed (e.g., by servlet-container thread-pool threads) which adds threadsafe
+     * constraint on some of the sub-builders.
+     * <p/>
+     * Sub-Builders that require thread-safety
+     * <pre><ul>
+     *     <li>{@link org.glassfish.jersey.server.internal.monitoring.ExecutionStatisticsImpl.Builder}</li>
+     *     <li>{@link org.glassfish.jersey.server.internal.monitoring.ResourceStatisticsImpl.Builder}</li>
+     *     <li>{@link org.glassfish.jersey.server.internal.monitoring.ResourceMethodStatisticsImpl.Builder}</li>
+     *     <li>{@link org.glassfish.jersey.server.internal.monitoring.TimeWindowStatisticsImpl.Builder}</li>
+     * </ul>
+     * The rest does not need to be thread-safe
+     * <ul>
+     *     <li>{@link org.glassfish.jersey.server.internal.monitoring.ExceptionMapperStatisticsImpl.Builder}</li>
+     *     <li>{@link org.glassfish.jersey.server.internal.monitoring.ResponseStatisticsImpl.Builder}</li>
+     * </ul></pre>
      */
     static class Builder {
 
@@ -90,7 +110,7 @@ public int compare(final Class<?> o1, final Class<?> o2) {
             }
         });
 
-        private ExecutionStatisticsImpl.Builder requestStatisticsBuilder;
+        private ExecutionStatisticsImpl.Builder executionStatisticsBuilder;
 
         /**
          * Create a new builder.
@@ -102,6 +122,7 @@ public int compare(final Class<?> o1, final Class<?> o2) {
 
         /**
          * Create a new builder and initialize it from resource model.
+         *
          * @param resourceModel resource model.
          */
         Builder(final ResourceModel resourceModel) {
@@ -143,6 +164,7 @@ private ResourceStatisticsImpl.Builder getOrCreateResourceBuilder(final Resource
 
         /**
          * Get the exception mapper statistics builder.
+         *
          * @return Builder of internal exception mapper statistics.
          */
         ExceptionMapperStatisticsImpl.Builder getExceptionMapperStatisticsBuilder() {
@@ -153,26 +175,25 @@ ExceptionMapperStatisticsImpl.Builder getExceptionMapperStatisticsBuilder() {
          * Add global request execution.
          *
          * @param startTime time of the execution.
-         * @param duration duration of the execution.
+         * @param duration  duration of the execution.
          */
         void addRequestExecution(final long startTime, final long duration) {
-            if (requestStatisticsBuilder == null) {
-                requestStatisticsBuilder = new ExecutionStatisticsImpl.Builder();
+            if (executionStatisticsBuilder == null) {
+                executionStatisticsBuilder = new ExecutionStatisticsImpl.Builder();
             }
-            requestStatisticsBuilder.addExecution(startTime, duration);
+            executionStatisticsBuilder.addExecution(startTime, duration);
         }
 
         /**
          * Add execution of a resource method.
          *
-         * @param uri String uri which was executed.
-         * @param resourceMethod Resource method.
-         * @param methodTime Time spent on execution of resource method itself (Unix timestamp format).
-         * @param methodDuration Time of execution of the resource method.
-         * @param requestTime Time of whole request processing (from receiving
-         *                    the request until writing the response). (Unix timestamp format)
-         * @param requestDuration Time when the request matching to the executed resource method has been received
-         *                         by Jersey.
+         * @param uri             String uri which was executed.
+         * @param resourceMethod  Resource method.
+         * @param methodTime      Time spent on execution of resource method itself (Unix timestamp format).
+         * @param methodDuration  Time of execution of the resource method.
+         * @param requestTime     Time of whole request processing (from receiving the request until writing the response). (Unix
+         *                        timestamp format)
+         * @param requestDuration Time when the request matching to the executed resource method has been received by Jersey.
          */
         void addExecution(final String uri, final ResourceMethod resourceMethod,
                           final long methodTime, final long methodDuration,
@@ -194,18 +215,18 @@ void addExecution(final String uri, final ResourceMethod resourceMethod,
                     .addResourceMethodExecution(methodTime, methodDuration, requestTime, requestDuration);
         }
 
-
         /**
          * Add a response status code produces by Jersey.
+         *
          * @param responseCode Response status code.
          */
         void addResponseCode(final int responseCode) {
             responseStatisticsBuilder.addResponseCode(responseCode);
         }
 
-
         /**
          * Build a new instance of monitoring statistics.
+         *
          * @return New instance of {@code MonitoringStatisticsImpl}.
          */
         MonitoringStatisticsImpl build() {
@@ -214,8 +235,8 @@ MonitoringStatisticsImpl build() {
             final Map<Class<?>, ResourceStatistics> classStats = Collections.unmodifiableMap(
                     Maps.transformValues(resourceClassStatistics, BUILDING_FUNCTION));
 
-            final ExecutionStatistics requestStats = requestStatisticsBuilder == null
-                    ? ExecutionStatisticsImpl.EMPTY : requestStatisticsBuilder.build();
+            final ExecutionStatistics requestStats = executionStatisticsBuilder == null
+                    ? ExecutionStatisticsImpl.EMPTY : executionStatisticsBuilder.build();
 
             return new MonitoringStatisticsImpl(
                     uriStats, classStats, requestStats,
@@ -230,7 +251,6 @@ MonitoringStatisticsImpl build() {
     private final Map<String, ResourceStatistics> uriStatistics;
     private final Map<Class<?>, ResourceStatistics> resourceClassStatistics;
 
-
     private MonitoringStatisticsImpl(final Map<String, ResourceStatistics> uriStatistics,
                                      final Map<Class<?>, ResourceStatistics> resourceClassStatistics,
                                      final ExecutionStatistics requestStatistics,
@@ -243,24 +263,31 @@ private MonitoringStatisticsImpl(final Map<String, ResourceStatistics> uriStatis
         this.exceptionMapperStatistics = exceptionMapperStatistics;
     }
 
-
     @Override
     public ExecutionStatistics getRequestStatistics() {
         return requestStatistics;
     }
 
-
     @Override
     public ResponseStatistics getResponseStatistics() {
         return responseStatistics;
     }
 
-
+    /**
+     * Refreshed (re-built) on every access. (uses {@link Maps#transformValues(Map, Function)})
+     *
+     * @return resource statistics
+     */
     @Override
     public Map<String, ResourceStatistics> getUriStatistics() {
         return uriStatistics;
     }
 
+    /**
+     * Refreshed (re-built) on every access. (uses {@link Maps#transformValues(Map, Function)})
+     *
+     * @return resource statistics
+     */
     @Override
     public Map<Class<?>, ResourceStatistics> getResourceClassStatistics() {
         return resourceClassStatistics;
@@ -273,7 +300,8 @@ public ExceptionMapperStatistics getExceptionMapperStatistics() {
 
     @Override
     public MonitoringStatistics snapshot() {
-        // snapshot functionality not yet implemented
+        // snapshot is not needed, this object is loosely immutable (see javadoc of Maps getters)
+        // all the other Statistics objects are immutable
         return this;
     }
 }