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: Simplify FeatureLoader by replacing the `Initial` inner class by an `initializer` field. Fix a resource leak: Stream was not closed if the `estimatedSize` was actually exact.
Date Wed, 06 Nov 2019 20:15:27 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 de47cb2  Simplify FeatureLoader by replacing the `Initial` inner class by an `initializer`
field. Fix a resource leak: Stream was not closed if the `estimatedSize` was actually exact.
de47cb2 is described below

commit de47cb21834a3c0522e1b899c5eee3a2fdbe124c
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Wed Nov 6 20:03:22 2019 +0100

    Simplify FeatureLoader by replacing the `Initial` inner class by an `initializer` field.
    Fix a resource leak: Stream was not closed if the `estimatedSize` was actually exact.
---
 .../org/apache/sis/gui/dataset/FeatureList.java    |  99 +++++++++++--
 .../org/apache/sis/gui/dataset/FeatureLoader.java  | 164 ++++++++++-----------
 .../org/apache/sis/gui/dataset/FeatureTable.java   |   3 +-
 3 files changed, 165 insertions(+), 101 deletions(-)

diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureList.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureList.java
index 2c384f3..9d4144e 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureList.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureList.java
@@ -25,6 +25,7 @@ import javafx.collections.ObservableListBase;
 import org.opengis.feature.Feature;
 import org.apache.sis.storage.FeatureSet;
 import org.apache.sis.internal.gui.BackgroundThreads;
+import org.apache.sis.internal.util.UnmodifiableArrayList;
 import org.apache.sis.util.ArraysExt;
 
 
@@ -33,11 +34,19 @@ import org.apache.sis.util.ArraysExt;
  * When an element is requested, if that element has not yet been read, the reading is done
  * in a background thread.
  *
+ * <p>This list is unmodifiable through public API. It can be modified only through
package-private API.
+ * This restriction exists for preventing uncontrolled modifications to introduce inconsistencies
with
+ * the modifications to be applied by the loader running in background thread.</p>
+ *
  * <p>This list does not accept null elements; any attempt to add a null feature is
silently ignored.
  * The null value is reserved for meaning that the element is in process of being loaded.</p>
  *
  * <p>All methods in this class shall be invoked from JavaFX thread.</p>
  *
+ * @todo Current implementation does not release previously loaded features.
+ *       We could do that in a previous version if memory usage is a problem,
+ *       provided that {@link Spliterator#ORDERED} is set.
+ *
  * @author  Martin Desruisseaux (Geomatys)
  * @version 1.1
  * @since   1.1
@@ -45,6 +54,23 @@ import org.apache.sis.util.ArraysExt;
  */
 final class FeatureList extends ObservableListBase<Feature> {
     /**
+     * Number of empty rows to show in the bottom of the table when we don't know how many
rows still
+     * need to be read. Those rows do not stay empty for long since they will become valid
as soon as
+     * the background loader finished to load a page ({@value FeatureLoader#PAGE_SIZE} rows).
+     */
+    private static final long NUM_PENDING_ROWS = 10;
+
+    /**
+     * Maximum number of rows that this list will allow.
+     */
+    private static final int MAXIMUM_ROWS = Integer.MAX_VALUE;
+
+    /**
+     * The {@link #elements} value when this list is empty.
+     */
+    private static final Feature[] EMPTY = new Feature[0];
+
+    /**
      * The elements in this list, never {@code null}.
      */
     private Feature[] elements;
@@ -81,7 +107,28 @@ final class FeatureList extends ObservableListBase<Feature> {
      * Creates a new list of features.
      */
     FeatureList() {
-        elements = new Feature[0];
+        elements = EMPTY;
+    }
+
+    /**
+     * Returns the currently valid elements.
+     */
+    private List<Feature> validElements() {
+        return UnmodifiableArrayList.wrap(elements, 0, validCount);
+    }
+
+    /**
+     * Clears the content of this list. This method shall be invoked when no loader is running
+     * in a background thread, or when the loaded decided itself to invoke this method.
+     */
+    final void clearUnsafe() {
+        final List<Feature> removed = validElements();
+        elements = EMPTY;
+        estimatedSize = 0;
+        validCount    = 0;
+        beginChange();
+        nextReplace(0, 0, removed);
+        endChange();
     }
 
     /**
@@ -101,10 +148,11 @@ final class FeatureList extends ObservableListBase<Feature> {
             previous.cancel();
         }
         if (features != null) {
-            nextPageLoader = new FeatureLoader.Initial(table, features);
+            nextPageLoader = new FeatureLoader(table, features);
             BackgroundThreads.execute(nextPageLoader);
             return true;
         } else {
+            clearUnsafe();
             return false;
         }
     }
@@ -112,35 +160,43 @@ final class FeatureList extends ObservableListBase<Feature> {
     /**
      * Invoked by {@link FeatureLoader} for replacing the current content by a new list of
features.
      * The list size after this method invocation will be {@code expectedSize}, not {@code
count}.
-     * The missing elements will be implicitly null until {@link #addFeatures(Feature[],
int)} is invoked.
-     * If the expected size is unknown (i.e. its value is {@link Long#MAX_VALUE}),
+     * The missing elements will be implicitly null until {@link #addFeatures(Feature[],
int, boolean)}
+     * is invoked. If the expected size is unknown (i.e. its value is {@link Long#MAX_VALUE}),
      * then an arbitrary size is computed from {@code count}.
      *
      * @param  remainingCount   value of {@link Spliterator#estimateSize()} after partial
traversal.
      * @param  characteristics  value of {@link Spliterator#characteristics()}.
      * @param  features         new features. This array is not cloned and may be modified
in-place.
      * @param  count            number of valid elements in the given array.
+     * @param  hasMore          if the stream may have more features.
      */
     @SuppressWarnings("AssignmentToCollectionOrArrayFieldFromParameter")
-    final void setFeatures(long remainingCount, int characteristics, final Feature[] features,
final int count) {
+    final void setFeatures(long remainingCount, int characteristics,
+                           final Feature[] features, final int count, final boolean hasMore)
+    {
         assert Platform.isFxApplicationThread();
         int newValidCount = 0;
         for (int i=0; i<count; i++) {
             final Feature f = features[i];
             if (f != null) features[newValidCount++] = f;       // Exclude null elements.
         }
-        final List<Feature> removed = Arrays.asList(elements);  // Want this call outside
{beginChange … endChange}.
+        final List<Feature> removed = validElements();          // Want this call outside
{beginChange … endChange}.
         if (remainingCount == Long.MAX_VALUE) {
-            remainingCount    = count + 10L;                    // Arbitrary additional amount.
+            remainingCount  = count + NUM_PENDING_ROWS;         // Arbitrary additional amount.
             characteristics = 0;
         }
-        estimatedSize = (int) Math.min(Integer.MAX_VALUE, Math.addExact(remainingCount, newValidCount));
+        estimatedSize = (int) Math.min(MAXIMUM_ROWS, Math.addExact(remainingCount, newValidCount));
         isSizeExact   = (characteristics & Spliterator.SIZED) != 0;
         elements      = features;
         validCount    = newValidCount;
+        if (hasMore && estimatedSize <= newValidCount) {
+            // Estimated size seems incorrect. Add some empty rows for triggering a new page
load.
+            estimatedSize = (int) Math.min(MAXIMUM_ROWS, newValidCount + NUM_PENDING_ROWS);
+        }
         beginChange();
         nextReplace(0, estimatedSize, removed);
         endChange();
+        checkOverflow();
     }
 
     /**
@@ -149,9 +205,10 @@ final class FeatureList extends ObservableListBase<Feature> {
      *
      * @param  features  the features to add. Null elements are ignored.
      * @param  count     number of valid elements in the given array.
+     * @param  hasMore   if the stream may have more features.
      * @throws ArithmeticException if the number of elements exceeds this list capacity.
      */
-    final void addFeatures(final Feature[] features, final int count) {
+    final void addFeatures(final Feature[] features, final int count, final boolean hasMore)
{
         assert Platform.isFxApplicationThread();
         if (count > 0) {
             int newValidCount = Math.addExact(validCount, count);
@@ -170,13 +227,29 @@ final class FeatureList extends ObservableListBase<Feature> {
              */
             final int replaceTo = Math.min(newValidCount, estimatedSize);
             final List<Feature> removed = Collections.nCopies(replaceTo - validCount,
null);
-            if (newValidCount > estimatedSize) {
+            if (newValidCount >= estimatedSize) {
                 estimatedSize = newValidCount;              // Update before we send events.
+                if (hasMore) {
+                    // Estimated size seems incorrect. Add some empty rows for triggering
a new page load.
+                    estimatedSize = (int) Math.min(MAXIMUM_ROWS, newValidCount + NUM_PENDING_ROWS);
+                }
             }
             beginChange();
             nextReplace(validCount, replaceTo, removed);
             nextAdd(replaceTo, validCount = newValidCount);
             endChange();
+            checkOverflow();
+        }
+    }
+
+    /**
+     * If we can not load more features stop the reading process.
+     *
+     * @todo Add some message in the widget for warning the user.
+     */
+    private void checkOverflow() {
+        if (validCount >= MAXIMUM_ROWS) {
+            interrupt();
         }
     }
 
@@ -189,6 +262,7 @@ final class FeatureList extends ObservableListBase<Feature> {
      */
     final void setNextPage(final FeatureLoader next) {
         assert Platform.isFxApplicationThread();
+        assert nextPageLoader.isDone();
         nextPageLoader = next;
         if (next == null) {
             final int n = estimatedSize - validCount;
@@ -234,8 +308,9 @@ final class FeatureList extends ObservableListBase<Feature> {
         if (isSizeExact && index >= estimatedSize) {
             throw new IndexOutOfBoundsException(index);
         }
-        if (nextPageLoader != null) {
-            BackgroundThreads.execute(nextPageLoader);
+        final FeatureLoader loader = nextPageLoader;
+        if (loader != null && loader.getState() == FeatureLoader.State.READY) {
+            BackgroundThreads.execute(loader);
         }
         return null;
     }
diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureLoader.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureLoader.java
index ba11213..2e6bc27 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureLoader.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureLoader.java
@@ -36,15 +36,12 @@ import org.apache.sis.internal.gui.Resources;
  * This task does not load all features; only {@value #PAGE_SIZE} of them are loaded.
  * The boolean value returned by this task tells whether there is more features to load.
  *
- * <p>Loading processes are started by {@link Initial} loader.
- * Only additional pages are loaded by ordinary {@code FeatureLoader}.</p>
- *
  * @author  Martin Desruisseaux (Geomatys)
  * @version 1.1
  * @since   1.1
  * @module
  */
-class FeatureLoader extends Task<Boolean> implements Consumer<Feature> {
+final class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
     /**
      * Maximum number of features to load in a background task.
      * If there is more features to load, we will use many tasks.
@@ -60,6 +57,13 @@ class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
     private final FeatureTable table;
 
     /**
+     * The feature set from which to get the initial configuration.
+     * This is non-null only for the task loading the first {@value #PAGE_SIZE} instances,
+     * then become null for all subsequent tasks.
+     */
+    private final FeatureSet initializer;
+
+    /**
      * The stream to close after we finished to iterate over features.
      * This stream should not be used for any other purpose.
      */
@@ -83,23 +87,11 @@ class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
     private int count;
 
     /**
-     * Creates a new loader. Callers shall invoke {@link #initialize(FeatureSet)}
-     * in the worker thread before to let the {@link #call()} method be executed.
-     *
-     * @see #initialize(FeatureSet)
+     * Creates a new loader for the given set of features.
      */
-    FeatureLoader(final FeatureTable table) {
-        this.table = table;
-    }
-
-    /**
-     * Initializes this task for reading features from the specified set.
-     * This method is invoked by subclasses after construction but before
-     * {@link #call()} execution.
-     */
-    final void initialize(final FeatureSet features) throws DataStoreException {
-        toClose  = features.features(false);
-        iterator = toClose.spliterator();
+    FeatureLoader(final FeatureTable table, final FeatureSet features) {
+        this.table  = table;
+        initializer = features;
     }
 
     /**
@@ -107,17 +99,10 @@ class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
      * The new task will load the next {@value #PAGE_SIZE} features.
      */
     private FeatureLoader(final FeatureLoader previous) {
-        table    = previous.table;
-        toClose  = previous.toClose;
-        iterator = previous.iterator;
-    }
-
-    /**
-     * Returns the list where to add features.
-     * All methods on the returned list shall be invoked from JavaFX thread.
-     */
-    private FeatureList destination() {
-        return (FeatureList) table.getItems();
+        table       = previous.table;
+        toClose     = previous.toClose;
+        iterator    = previous.iterator;
+        initializer = null;
     }
 
     /**
@@ -133,23 +118,39 @@ class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
      * Invoked in a background thread for loading up to {@value #PAGE_SIZE} features.
      * If this method completed successfully but there is still more feature to read,
      * then {@link #iterator} will keep a non-null value and a new {@link FeatureLoader}
-     * should be prepared for reading of another page of features. In other cases,
-     * {@link #iterator} is null and the stream has been closed.
+     * well be prepared by {@link #succeeded()} for reading of another page of features.
+     * In other cases, {@link #iterator} is null and the stream has been closed.
      *
      * @return whether there is more features to load.
      */
     @Override
     protected Boolean call() throws DataStoreException {
-        // Note: iterator.estimateSize() is a count or remaining elements.
-        final int stopAt = (int) Math.min(iterator.estimateSize(), PAGE_SIZE);
+        final boolean isTypeKnown;
+        if (initializer != null) {
+            isTypeKnown = setType(initializer.getType());
+            toClose     = initializer.features(false);
+            iterator    = toClose.spliterator();
+        } else {
+            isTypeKnown = true;
+        }
+        /*
+         * iterator.estimateSize() is a count or remaining elements (not the total number).
+         * If the number of remaining elements is equal to smaller than the page size, try
+         * to read one more element in order to check if we really reached the stream end.
+         * We do that because the estimated count is only approximate.
+         */
+        final long remaining = iterator.estimateSize();
+        final int stopAt = (remaining > PAGE_SIZE) ? PAGE_SIZE : 1 + (int) remaining;
         loaded = new Feature[stopAt];
         try {
             while (iterator.tryAdvance(this)) {
                 if (count >= stopAt) {
+                    setMissingType(isTypeKnown);
                     return Boolean.TRUE;                // Intentionally skip the call to
close().
                 }
                 if (isCancelled()) {
-                    break;
+                    close();
+                    return Boolean.FALSE;
                 }
             }
         } catch (BackingStoreException e) {
@@ -160,7 +161,8 @@ class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
             }
             throw e.unwrapOrRethrow(DataStoreException.class);
         }
-        close();                                        // Loading completed or has been
cancelled.
+        close();                                        // Loading completed successfully.
+        setMissingType(isTypeKnown);
         return Boolean.FALSE;
     }
 
@@ -213,20 +215,38 @@ class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
     }
 
     /**
+     * Returns the list where to add features.
+     * All methods on the returned list shall be invoked from JavaFX thread.
+     */
+    private FeatureList destination() {
+        return (FeatureList) table.getItems();
+    }
+
+    /**
      * Invoked in JavaFX thread after new feature instances are ready.
      * This method adds the new rows in the table and prepares another
      * task for loading the next batch of features when needed.
      */
     @Override
-    protected final void succeeded() {
+    protected void succeeded() {
         final FeatureList addTo = destination();
         if (addTo.isCurrentLoader(this)) {
-            if (this instanceof Initial) {
-                addTo.setFeatures(iterator.estimateSize(), iterator.characteristics(), loaded,
count);
+            final boolean hasMore = getValue();
+            if (initializer != null) {
+                final long remainingCount;
+                final int characteristics;
+                if (hasMore) {
+                    remainingCount  = iterator.estimateSize();
+                    characteristics = iterator.characteristics();
+                } else {
+                    remainingCount  = 0;
+                    characteristics = Spliterator.SIZED;
+                }
+                addTo.setFeatures(remainingCount, characteristics, loaded, count, hasMore);
             } else {
-                addTo.addFeatures(loaded, count);
+                addTo.addFeatures(loaded, count, hasMore);
             }
-            addTo.setNextPage(getValue() ? new FeatureLoader(this) : null);
+            addTo.setNextPage(hasMore ? new FeatureLoader(this) : null);
         } else try {
             close();
         } catch (DataStoreException e) {
@@ -242,7 +262,7 @@ class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
      * @see FeatureTable#interrupt()
      */
     @Override
-    protected final void cancelled() {
+    protected void cancelled() {
         final FeatureList addTo = destination();
         final boolean isCurrentLoader = addTo.isCurrentLoader(this);
         if (isCurrentLoader) {
@@ -277,55 +297,23 @@ class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
      * Invoked in JavaFX thread when a loading process failed.
      */
     @Override
-    protected final void failed() {
+    protected void failed() {
         cancelled();
     }
 
     /**
-     * The task to execute in background thread for initiating the loading process.
-     * This tasks is created only for the first {@value #PAGE_SIZE} features.
-     * For all additional features, an ordinary {@link FeatureLoader} will be used.
-     */
-    static final class Initial extends FeatureLoader {
-        /**
-         * The set of features to read.
-         */
-        private final FeatureSet features;
-
-        /**
-         * Initializes a new task for loading features from the given set.
-         */
-        Initial(final FeatureTable table, final FeatureSet features) {
-            super(table);
-            this.features = features;
-        }
-
-        /**
-         * Gets the feature type, initializes the iterator and gets the first {@value #PAGE_SIZE}
features.
-         * The {@link FeatureType} should be given by {@link FeatureSet#getType()} but this
method is robust
-         * to incomplete implementations where {@code getType()} returns {@code null}.
-         */
-        @Override
-        protected Boolean call() throws DataStoreException {
-            final boolean isTypeKnown = setType(features.getType());
-            initialize(features);
-            final Boolean status = super.call();
-            if (isTypeKnown) {
-                setTypeFromFirst();
-            }
-            return status;
-        }
-    }
-
-    /**
      * Invoked when the feature type may have been found. If the given type is non-null,
      * then this method delegates to {@link FeatureTable#setFeatureType(FeatureType)} in
      * the JavaFX thread. This will erase the previous content and prepare new columns.
      *
+     * <p>This method is invoked, directly or indirectly, only from the {@link #call()}
+     * method with non-null {@link #initializer}. Consequently the new rows have not yet
+     * been added at this time.</p>
+     *
      * @param  type  the feature type, or {@code null}.
      * @return whether the given type was non-null.
      */
-    final boolean setType(final FeatureType type) {
+    private boolean setType(final FeatureType type) {
         if (type != null) {
             Platform.runLater(() -> table.setFeatureType(type));
             return true;
@@ -340,13 +328,15 @@ class FeatureLoader extends Task<Boolean> implements Consumer<Feature>
{
      * incomplete implementations exist so we are better to be safe. If we can not get the
type
      * from the first feature instances, we will give up.
      */
-    final void setTypeFromFirst() throws DataStoreException {
-        for (int i=0; i<count; i++) {
-            final Feature f = loaded[i];
-            if (f != null && setType(f.getType())) {
-                return;
+    private void setMissingType(final boolean isTypeKnown) throws DataStoreException {
+        if (!isTypeKnown) {
+            for (int i=0; i<count; i++) {
+                final Feature f = loaded[i];
+                if (f != null && setType(f.getType())) {
+                    return;
+                }
             }
+            throw new DataStoreException(Resources.forLocale(table.textLocale).getString(Resources.Keys.NoFeatureTypeInfo));
         }
-        throw new DataStoreException(Resources.forLocale(table.textLocale).getString(Resources.Keys.NoFeatureTypeInfo));
     }
 }
diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureTable.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureTable.java
index 909cf88..c26e143 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureTable.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/dataset/FeatureTable.java
@@ -99,7 +99,6 @@ public class FeatureTable extends TableView<Feature> {
         final FeatureList items = (FeatureList) getItems();
         if (!items.setFeatures(this, features)) {
             featureType = null;
-            items.clear();
             getColumns().clear();
         }
     }
@@ -110,7 +109,7 @@ public class FeatureTable extends TableView<Feature> {
      * determined from the given type.
      */
     final void setFeatureType(final FeatureType type) {
-        getItems().clear();
+        ((FeatureList) getItems()).clearUnsafe();
         if (type != null && !type.equals(featureType)) {
             final Collection<? extends PropertyType> properties = type.getProperties(true);
             final List<TableColumn<Feature,?>> columns = new ArrayList<>(properties.size());


Mime
View raw message