sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] branch geoapi-4.0 updated: Clarify which `LogRecord` properties are initialised when an error is reported.
Date Thu, 04 Feb 2021 15:22:47 GMT
This is an automated email from the ASF dual-hosted git repository.

desruisseaux pushed a commit to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git


The following commit(s) were added to refs/heads/geoapi-4.0 by this push:
     new d27acc8  Clarify which `LogRecord` properties are initialised when an error is reported.
d27acc8 is described below

commit d27acc8bc457a664fd33e547d74585082e0f4caf
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Thu Feb 4 12:54:08 2021 +0100

    Clarify which `LogRecord` properties are initialised when an error is reported.
---
 .../java/org/apache/sis/image/AnnotatedImage.java  | 73 ++++++++++---------
 .../java/org/apache/sis/image/ErrorAction.java     |  1 +
 .../java/org/apache/sis/image/ErrorHandler.java    | 82 +++++++++++++++++-----
 .../sis/internal/coverage/j2d/TileOpExecutor.java  | 32 +++++++--
 4 files changed, 134 insertions(+), 54 deletions(-)

diff --git a/core/sis-feature/src/main/java/org/apache/sis/image/AnnotatedImage.java b/core/sis-feature/src/main/java/org/apache/sis/image/AnnotatedImage.java
index 7d38278..8bbae85 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/image/AnnotatedImage.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/image/AnnotatedImage.java
@@ -326,39 +326,39 @@ abstract class AnnotatedImage extends ImageAdapter {
              */
             final Object key = getCacheKey(property);
             value = cache.peek(key);
-            if (value == null) try {
+            if (value == null) {
                 boolean success = false;
                 final Cache.Handler<Object> handler = cache.lock(key);
                 try {
                     value = handler.peek();
-                    if (value == null) {
+                    if (value == null) try {
                         value = computeProperty();
                         if (value == null) value = NULL;
                         success = (errors == null);
+                    } catch (Exception e) {
+                        /*
+                         * Stores the given exception in a log record. We use a log record
in order to initialize
+                         * the timestamp and thread ID to the values they had at the time
the first error occurred.
+                         */
+                        if (failOnException) {
+                            throw (ImagingOpException) new ImagingOpException(
+                                    Errors.format(Errors.Keys.CanNotCompute_1, property)).initCause(e);
+                        }
+                        synchronized (this) {
+                            ErrorHandler.Report report = errors;
+                            final boolean create = (report == null);
+                            if (create) {
+                                report = new ErrorHandler.Report();
+                            }
+                            report.addPropertyError(e, property);
+                            if (create) setError(report);
+                        }
                     }
                 } finally {
                     handler.putAndUnlock(success ? value : null);       // Cache only if
no error occurred.
                 }
                 if (value == NULL) value = null;
                 else value = cloneProperty(property, value);
-            } catch (Exception e) {
-                /*
-                 * Stores the given exception in a log record. We use a log record in order
to initialize
-                 * the timestamp and thread ID to the values they had at the time the first
error occurred.
-                 */
-                if (failOnException) {
-                    throw (ImagingOpException) new ImagingOpException(
-                            Errors.format(Errors.Keys.CanNotCompute_1, property)).initCause(e);
-                }
-                synchronized (this) {
-                    ErrorHandler.Report report = errors;
-                    final boolean create = (report == null);
-                    if (create) {
-                        report = new ErrorHandler.Report();
-                    }
-                    report.addPropertyError(e, property);
-                    if (create) setError(report);
-                }
             }
         } else if (isErrorProperty(property, name)) {
             value = errors;
@@ -371,7 +371,8 @@ abstract class AnnotatedImage extends ImageAdapter {
     /**
      * Invoked by {@link TileOpExecutor} if an error occurred during calculation on a tiles.
      * Can also be invoked by {@link #getProperty(String)} directly if the error occurred
-     * outside {@link TileOpExecutor}. This method shall be invoked at most once.
+     * outside {@link TileOpExecutor}. This method should be invoked at most once, unless
+     * the calculation is attempted again.
      *
      * @param  report  a description of the error that occurred.
      */
@@ -388,9 +389,16 @@ abstract class AnnotatedImage extends ImageAdapter {
     }
 
     /**
-     * If an error occurred, logs the message. The log record is cleared by this method call
+     * If an error occurred, logs the message with the specified class and method as the
source.
+     * The {@code classe} and {@code method} arguments overwrite the {@link LogRecord#getSourceClassName()}
+     * and {@link LogRecord#getSourceMethodName()} values. The log record is cleared by this
method call
      * and will no longer be reported, unless the property is recomputed.
      *
+     * <h4>Context of use</h4>
+     * This method should be invoked only on images that are going to be disposed after the
caller extracted
+     * the computed property value. This method should not be invoked on image accessible
by the user,
+     * because clearing the error may be surprising.
+     *
      * @param  classe   the class to report as the source of the logging message.
      * @param  method   the method to report as the source of the logging message.
      * @param  handler  where to send the log message.
@@ -399,16 +407,15 @@ abstract class AnnotatedImage extends ImageAdapter {
         final ErrorHandler.Report report;
         synchronized (this) {
             report = errors;
-            errors = null;
-        }
-        if (report != null) {
-            final LogRecord record = report.getDescription();
-            if (record != null) {
-                record.setSourceClassName(classe.getCanonicalName());
-                record.setSourceMethodName(method);
-                handler.handle(report);
+            if (report == null || report.isEmpty()) {
+                return;
             }
+            final LogRecord record = report.getDescription();
+            record.setSourceClassName(classe.getCanonicalName());
+            record.setSourceMethodName(method);
+            errors = null;          // Make sure that no other thread will use that `Report`
instance.
         }
+        handler.handle(report);
     }
 
     /**
@@ -431,12 +438,12 @@ abstract class AnnotatedImage extends ImageAdapter {
     protected Object computeProperty() throws Exception {
         if (parallel) {
             final TileOpExecutor executor = new TileOpExecutor(source, boundsOfInterest);
-            if (!failOnException) {
-                executor.setErrorHandler(this::setError);
-            }
             if (executor.isMultiTiled()) {
                 final Collector<? super Raster,?,?> collector = collector();
                 if (collector != null) {
+                    if (!failOnException) {
+                        executor.setErrorHandler(this::setError);
+                    }
                     return executor.executeOnReadable(source, collector());
                 }
             }
diff --git a/core/sis-feature/src/main/java/org/apache/sis/image/ErrorAction.java b/core/sis-feature/src/main/java/org/apache/sis/image/ErrorAction.java
index b2abedd..d441f00 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/image/ErrorAction.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/image/ErrorAction.java
@@ -62,6 +62,7 @@ enum ErrorAction implements ErrorHandler {
                 String logger = record.getLoggerName();
                 if (logger == null) {
                     logger = Modules.RASTER;
+                    record.setLoggerName(logger);
                 }
                 Logging.getLogger(logger).log(record);
             } else {
diff --git a/core/sis-feature/src/main/java/org/apache/sis/image/ErrorHandler.java b/core/sis-feature/src/main/java/org/apache/sis/image/ErrorHandler.java
index ba20674..0c940b6 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/image/ErrorHandler.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/image/ErrorHandler.java
@@ -19,6 +19,7 @@ package org.apache.sis.image;
 import java.awt.Point;
 import java.util.Arrays;
 import java.util.Locale;
+import java.util.Objects;
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
 import java.awt.image.ImagingOpException;
@@ -99,13 +100,48 @@ public interface ErrorHandler {
         }
 
         /**
+         * Returns {@code true} if the given exceptions are of the same class, have the same
message
+         * and the same stack trace. The cause and the suppressed exceptions are ignored.
+         */
+        private static boolean equals(final Throwable t1, final Throwable t2) {
+            return t1.getClass() == t2.getClass()
+                    && Objects.equals(t1.getMessage(),    t2.getMessage())
+                    &&  Arrays.equals(t1.getStackTrace(), t2.getStackTrace());
+        }
+
+        /**
+         * Adds the {@code more} exception to the list if suppressed exceptions if not already
present.
+         */
+        private static void addSuppressed(final Throwable error, final Throwable more) {
+            if (equals(error, more)) return;
+            for (final Throwable s : error.getSuppressed()) {
+                if (equals(s, more)) return;
+            }
+            error.addSuppressed(more);
+        }
+
+        /**
          * Reports an error that occurred while computing an image property.
          * This method can be invoked many times on the same {@code Report} instance.
          *
-         * @param error     the error that occurred.
-         * @param property  name of the property which was computed, or {@code null} if none.
+         * <h4>Logging information</h4>
+         * {@code Report} creates a {@link LogRecord} the first time that an {@code add(…)}
method is invoked.
+         * The record will have its
+         * {@linkplain LogRecord#getLevel() level},
+         * {@linkplain LogRecord#getMessage() message} and
+         * {@linkplain LogRecord#getThrown() exception} properties initialized. But the
+         * {@linkplain LogRecord#getSourceClassName() source class name},
+         * {@linkplain LogRecord#getSourceMethodName() source method name} and
+         * {@linkplain LogRecord#getLoggerName() logger name} may be undefined;
+         * they may need to be completed by the caller.
+         *
+         * @param  error     the error that occurred.
+         * @param  property  name of the property which was computed, or {@code null} if
none.
+         * @return {@code true} if this is the first time that an error is reported
+         *         (in which case a {@link LogRecord} instance has been created),
+         *         or {@code false} if a {@link LogRecord} already exists.
          */
-        public void addPropertyError(final Throwable error, final String property) {
+        public boolean addPropertyError(final Throwable error, final String property) {
             ArgumentChecks.ensureNonNull("error", error);
             if (description == null) {
                 if (property != null) {
@@ -115,8 +151,10 @@ public interface ErrorHandler {
                     description = new LogRecord(Level.WARNING, error.toString());
                 }
                 description.setThrown(error);
+                return true;
             } else {
-                description.getThrown().addSuppressed(error);
+                addSuppressed(description.getThrown(), error);
+                return false;
             }
         }
 
@@ -124,11 +162,25 @@ public interface ErrorHandler {
          * Reports an error that occurred while computing an image tile.
          * This method can be invoked many times on the same {@code Report} instance.
          *
-         * @param error  the error that occurred.
-         * @param tx     column index of the tile where the error occurred.
-         * @param ty     row index of the tile where the error occurred.
+         * <h4>Logging information</h4>
+         * {@code Report} creates a {@link LogRecord} the first time that an {@code add(…)}
method is invoked.
+         * The record will have its
+         * {@linkplain LogRecord#getLevel() level},
+         * {@linkplain LogRecord#getMessage() message} and
+         * {@linkplain LogRecord#getThrown() exception} properties initialized. But the
+         * {@linkplain LogRecord#getSourceClassName() source class name},
+         * {@linkplain LogRecord#getSourceMethodName() source method name} and
+         * {@linkplain LogRecord#getLoggerName() logger name} may be undefined;
+         * they may need to be completed by the caller.
+         *
+         * @param  error  the error that occurred.
+         * @param  tx     column index of the tile where the error occurred.
+         * @param  ty     row index of the tile where the error occurred.
+         * @return {@code true} if this is the first time that an error is reported
+         *         (in which case a {@link LogRecord} instance has been created),
+         *         or {@code false} if a {@link LogRecord} already exists.
          */
-        public void addTileError(final Throwable error, final int tx, final int ty) {
+        public boolean addTileError(final Throwable error, final int tx, final int ty) {
             ArgumentChecks.ensureNonNull("error", error);
             if (indices == null) {
                 indices = new int[8];
@@ -141,8 +193,10 @@ public interface ErrorHandler {
                 description = Resources.forLocale(null)
                         .getLogRecord(Level.WARNING, Resources.Keys.CanNotProcessTile_2,
tx, ty);
                 description.setThrown(error);
+                return true;
             } else {
-                description.getThrown().addSuppressed(error);
+                addSuppressed(description.getThrown(), error);
+                return false;
             }
         }
 
@@ -160,14 +214,8 @@ public interface ErrorHandler {
         }
 
         /**
-         * Returns a description of errors as a log record. The exception can be obtained
by
-         * {@link LogRecord#getThrown()}. In addition the {@code LogRecord} has the
-         * {@linkplain LogRecord#getLevel() level} and
-         * {@linkplain LogRecord#getMessage() message} properties set. But the
-         * {@linkplain LogRecord#getSourceClassName() source class name},
-         * {@linkplain LogRecord#getSourceMethodName() source method name} and
-         * {@linkplain LogRecord#getLoggerName() logger name} may be undefined;
-         * they should be set by the {@link ErrorHandler}.
+         * Returns a description of errors as a log record.
+         * The exception can be obtained by {@link LogRecord#getThrown()}.
          * The return value is never null unless this report {@linkplain #isEmpty() is empty}.
          *
          * @return errors description, or {@code null} if this report is empty.
diff --git a/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/j2d/TileOpExecutor.java
b/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/j2d/TileOpExecutor.java
index bed6e80..c81ca58 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/j2d/TileOpExecutor.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/j2d/TileOpExecutor.java
@@ -81,8 +81,10 @@ public class TileOpExecutor {
 
     /**
      * Where to report exceptions, or {@link ErrorHandler#THROW} for throwing them.
-     * In current implementation this is used only during parallel computation.
-     * A future version may need to use it for sequential computations as well for consistency.
+     * If at least one error occurred, then this handler will receive the {@link Cursor#errors}
report
+     * after all computation {@linkplain Cursor#finish finished}.
+     *
+     * @see #setErrorHandler(ErrorHandler)
      */
     private ErrorHandler errorHandler;
 
@@ -116,6 +118,17 @@ public class TileOpExecutor {
 
     /**
      * Sets the handler where to report exceptions.
+     * The exception can be obtained by {@link LogRecord#getThrown()}
+     * on the value returned by {@link ErrorHandler.Report#getDescription()}.
+     * In addition the {@code LogRecord} will have the
+     * {@linkplain LogRecord#getLevel() level} and
+     * {@linkplain LogRecord#getMessage() message} properties set. But the
+     * {@linkplain LogRecord#getSourceClassName() source class name},
+     * {@linkplain LogRecord#getSourceMethodName() source method name} and
+     * {@linkplain LogRecord#getLoggerName() logger name} will be undefined;
+     * they should be set by the given {@link ErrorHandler}.
+     *
+     * <h4>Limitation</h4>
      * In current implementation this is used only during parallel computation.
      * A future version may need to use it for sequential computations as well for consistency.
      *
@@ -537,7 +550,18 @@ public class TileOpExecutor {
 
         /**
          * The errors that occurred while computing a tile.
-         * Will be ignored if {@linkplain ErrorHandler.Report#isEmpty() empty}.
+         * If this report {@linkplain ErrorHandler.Report#isEmpty() is empty},
+         * then it will be ignored. Otherwise it contains a log record with
+         * {@linkplain LogRecord#getLevel() level},
+         * {@linkplain LogRecord#getMessage() message} and
+         * {@linkplain LogRecord#getThrown() exception} properties set. But the
+         * {@linkplain LogRecord#getSourceClassName() source class name},
+         * {@linkplain LogRecord#getSourceMethodName() source method name} and
+         * {@linkplain LogRecord#getLoggerName() logger name} will be undefined.
+         *
+         * <p>If the report is non-empty, it will be given to the
+         * {@linkplain TileOpExecutor#errorHandler error handler}
+         * after all computation {@linkplain #finish finished}.</p>
          *
          * @see #stopOnError
          * @see #recordError(Worker, Throwable)
@@ -871,7 +895,7 @@ public class TileOpExecutor {
         static <A,R> R execute(final TileOpExecutor executor, final RenderedImage source,
                 final Collector<? super Raster, A, R> collector, final ErrorHandler
errorHandler)
         {
-            final Cursor<RenderedImage,A> cursor = executor.new Cursor<>(source,
collector, errorHandler == null);
+            final Cursor<RenderedImage,A> cursor = executor.new Cursor<>(source,
collector, errorHandler == ErrorHandler.THROW);
             final Future<?>[] workers = new Future<?>[cursor.getNumWorkers()];
             for (int i=0; i<workers.length; i++) {
                 workers[i] = CommonExecutor.instance().submit(new ReadWork<>(cursor,
collector));


Mime
View raw message