Stop Android storage entries coming back empty after an abrupt shutdown

CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java

@@ -6800,6 +6800,58 @@ public void setStorageData(Object storageData) {
     /// - `name`: the name of the storage file
     public abstract void deleteStorageFile(String name);
 
+    /// Creates an output stream that holds the whole new value and only becomes the
+    /// entry once it is closed, for an implementation that can offer that.
+    ///
+    /// This is not what `createStorageOutputStream` does, and the two are kept apart
+    /// on purpose. That one backs a public streaming API: a caller may hold it open
+    /// for the life of the application and expect what it has flushed to be readable
+    /// meanwhile, which is exactly how the log writer uses it. An entry that appears
+    /// only on close would leave such a caller writing to something nothing can read.
+    ///
+    /// Writing a whole value at once has no such expectation, and gains what the
+    /// streaming form cannot be given: the entry is never seen partly written, and
+    /// what was there before survives a write that fails.
+    ///
+    /// #### Parameters
+    ///
+    /// - `name`: the storage file name
+    ///
+    /// #### Returns
+    ///
+    /// an output stream, which replaces the entry as one step when closed if this
+    /// implementation is able to
+    public OutputStream createStorageOutputStream(String name, boolean replaceWhenClosed) throws IOException {
+        return createStorageOutputStream(name);
+    }
+
+    /// Gives up a write that failed partway through, for an implementation that can
+    /// throw one away without the entry it was replacing being any the worse for it.
+    ///
+    /// An implementation that writes into the entry itself leaves half an object
+    /// behind when a write fails, and the only way to be rid of that is to delete the
+    /// entry, so the default here reports that it cannot help. One that prepares the
+    /// new value elsewhere and puts it in place in a single step has not touched the
+    /// entry at all, and deleting it would throw away a good value on account of a
+    /// write that never reached it.
+    ///
+    /// #### Parameters
+    ///
+    /// - `name`: the name of the storage file being written
+    ///
+    /// - `writing`: the stream this implementation returned for the write that
+    /// failed, or null if it never opened. The write is named by its stream rather
+    /// than by its entry so that a second write to the same entry, which may be
+    /// perfectly healthy, is not given up along with it
+    ///
+    /// #### Returns
+    ///
+    /// true if the pending write was discarded and the entry left as it was, false if
+    /// the caller still has to delete the entry to be rid of a partial write
+    public boolean abandonStorageWrite(String name, OutputStream writing) {
+        return false;
+    }
+
     /// Deletes all the files in the application storage
     public void clearStorage() {
         String[] l = listStorageEntries();

CodenameOne/src/com/codename1/io/Preferences.java

@@ -107,13 +107,21 @@ private static synchronized void save() {
     ///
     /// - `o`: a String a number or boolean
     private static void set(String pref, Object o) {
-        Object prior = get(pref, null);
-        if (o == null) {
-            get().remove(pref);
-        } else {
-            get().put(pref, o);
+        Object prior;
+        // the change and the save that persists it have to be one step. save()
+        // serializes the map by writing an entry count and then walking it, so a
+        // change landing from another thread in between wrote a file whose count did
+        // not match its contents, and every preference in it read back as garbage.
+        // Listeners fire outside the lock, they are free to call back in here.
+        synchronized (Preferences.class) {
+            prior = get(pref, null);
+            if (o == null) {
+                get().remove(pref);
+            } else {
+                get().put(pref, o);
+            }
+            save();
         }
-        save();
         fireChange(pref, prior, o);
     }
 
@@ -124,18 +132,20 @@ private static void set(String pref, Object o) {
     /// - `values`: The key/value pairs to set in preferences.
     public static void set(Map<String, Object> values) {
         ArrayList<Object[]> changeParams = new ArrayList<Object[]>();
-        for (Map.Entry<String, Object> entry : values.entrySet()) {
-            String pref = entry.getKey();
-            Object o = entry.getValue();
-            Object prior = get(pref, null);
-            if (o == null) {
-                get().remove(pref);
-            } else {
-                get().put(pref, o);
+        synchronized (Preferences.class) {
+            for (Map.Entry<String, Object> entry : values.entrySet()) {
+                String pref = entry.getKey();
+                Object o = entry.getValue();
+                Object prior = get(pref, null);
+                if (o == null) {
+                    get().remove(pref);
+                } else {
+                    get().put(pref, o);
+                }
+                changeParams.add(new Object[]{pref, prior, o});
             }
-            changeParams.add(new Object[]{pref, prior, o});
+            save();
         }
-        save();
         for (Object[] params : changeParams) {
             fireChange((String) params[0], params[1], params[2]);
         }
@@ -202,31 +212,36 @@ public static void set(String pref, float f) {
     ///
     /// - `pref`: the preference value
     public static void delete(String pref) {
-        Object prior = get(pref, null);
-        get().remove(pref);
-        save();
+        Object prior;
+        synchronized (Preferences.class) {
+            prior = get(pref, null);
+            get().remove(pref);
+            save();
+        }
         fireChange(pref, prior, null);
     }
 
     /// Remove all preferences
     public static void clearAll() {
         // We only need to save prior values for Preferences that actually have listeners.
         Hashtable<String, Object> priorValues = null;
-        if (!listenerMap.isEmpty()) {
-
-            // Save all the Preferences for which there are registered listeners.
-            priorValues = new Hashtable<String, Object>();
-            for (String key : listenerMap.keySet()) {
-                final Object currentValue = get().get(key);
-                // We can't put null values in the hashtable. But we don't need to, because if we could we'd just be calling 
-                // fireChange(Pref, null, null) and fireChange would do nothing.
-                if (currentValue != null) {
-                    priorValues.put(key, currentValue);
+        synchronized (Preferences.class) {
+            if (!listenerMap.isEmpty()) {
+
+                // Save all the Preferences for which there are registered listeners.
+                priorValues = new Hashtable<String, Object>();
+                for (String key : listenerMap.keySet()) {
+                    final Object currentValue = get().get(key);
+                    // We can't put null values in the hashtable. But we don't need to, because if we could we'd just be calling
+                    // fireChange(Pref, null, null) and fireChange would do nothing.
+                    if (currentValue != null) {
+                        priorValues.put(key, currentValue);
+                    }
                 }
             }
+            get().clear();
+            save();
         }
-        get().clear();
-        save();
         if (priorValues != null) {
             for (String key : listenerMap.keySet()) {
                 fireChange(key, priorValues.get(key), null);

CodenameOne/src/com/codename1/io/Storage.java

@@ -471,25 +471,118 @@ public boolean writeObject(String name, Object o) {
     public boolean writeObject(String name, Object o, boolean includeLogging) {
         name = fixFileName(name);
         cache.put(name, o);
+        OutputStream writing = null; //NOPMD CloseResource
         DataOutputStream d = null; //NOPMD CloseResource
         try {
-            d = new DataOutputStream(createOutputStream(name));
+            // the stream is kept, because that is what names the write to be given up
+            // if this one fails. Giving up by entry name would reach every write to
+            // that entry, and a second thread writing the same one would be told its
+            // value was stored after it had been discarded.
+            writing = createOutputStreamForWrite(name);
+            d = new DataOutputStream(writing);
             Util.writeObject(o, d);
+            // closed here rather than left to the finally, because closing is where
+            // an implementation that writes the entry in one step does the writing.
+            // From the finally the failure would reach cleanup(), which logs and
+            // swallows it, and this method would report a write that never landed.
+            // handed off so the finally does not close it a second time
+            DataOutputStream closing = d; //NOPMD CloseResource
+            d = null;
+            closing.close();
             return true;
+            // Errors are caught too, and rethrown. An OutOfMemoryError partway
+            // through a large object would otherwise leave the entry to the finally,
+            // which closes the stream, and an implementation that publishes an entry
+            // as it closes would put those few bytes in place of a good entry. The
+            // error still reaches the caller, it just does not take the entry with it.
+        } catch (Error err) {
+            failedWrite(name, writing, err, includeLogging);
+            throw err;
         } catch (Exception err) {
-            if (includeLogging) {
-                Log.e(err);
-                if (Log.isCrashBound()) {
-                    Log.sendLog();
-                }
-            }
-            Util.getImplementation().deleteStorageFile(name);
+            failedWrite(name, writing, err, includeLogging);
             return false;
         } finally {
             Util.getImplementation().cleanup(d);
         }
     }
 
+    /// Creates the stream that `writeObject` writes a whole value into.
+    ///
+    /// This is deliberately not `createOutputStream`. That one is the public
+    /// streaming API, where a caller may hold the stream open and read back what it
+    /// has flushed -- the log writer keeps one for the life of the application -- so
+    /// it goes on writing into the entry. A whole value has no such expectation, and
+    /// so can be given what streaming cannot: it is assembled away from the entry and
+    /// put in place as one step, where the platform is able to, so the entry is never
+    /// seen half written and a write that fails leaves what was stored alone.
+    ///
+    /// A `Storage` installed through `setStorageInstance` to wrap the bytes -- the
+    /// seamless encryption that extension point exists for -- has always had
+    /// `writeObject` go through its own `createOutputStream`, and still does:
+    /// bypassing it would write those bytes past the encryption while reads went on
+    /// expecting it. Such a subclass can override this method to take the stronger
+    /// guarantee as well, wrapping what the superclass returns.
+    ///
+    /// #### Parameters
+    ///
+    /// - `name`: the storage file name, already normalized
+    ///
+    /// #### Returns
+    ///
+    /// the stream to write the value into
+    protected OutputStream createOutputStreamForWrite(String name) throws IOException {
+        if (getClass() != Storage.class) {
+            return createOutputStream(name);
+        }
+        return Util.getImplementation().createStorageOutputStream(name, true);
+    }
+
+    /// Gives up on a write that failed partway through, leaving neither a partial
+    /// entry on the storage nor the object that never reached it in the cache.
+    ///
+    /// #### Parameters
+    ///
+    /// - `name`: the storage file name, already normalized
+    ///
+    /// - `writing`: the implementation's stream for this write, or null if it never
+    /// opened
+    ///
+    /// - `err`: what went wrong
+    ///
+    /// - `includeLogging`: whether the failure may be logged, which is unsafe during
+    /// app initialization
+    private void failedWrite(String name, OutputStream writing, Throwable err,
+            boolean includeLogging) {
+        // before the logging, which can fail in its own right. Reporting an
+        // OutOfMemoryError means building a message and a stack trace, so a second
+        // failure there would carry off the rest of this method, and the write left
+        // open would be published by the finally that closes it.
+        //
+        // The cached copy goes either way. Leaving it behind hid the failure for the
+        // rest of the session: every read was answered from memory with the object
+        // that never reached the storage, and the entry only turned up missing after
+        // the app was restarted.
+        //
+        // Whether the entry itself has to go depends on where the failed write went.
+        // An implementation that writes into the entry has left half an object there
+        // and deleting is the only way to be rid of it. One that assembles the value
+        // elsewhere and puts it in place in a single step never touched the entry, so
+        // deleting it would answer a write that failed by throwing away the value
+        // that was already stored -- which is worse than the failure itself, and is
+        // what running out of memory partway through a large object used to do.
+        if (Util.getImplementation().abandonStorageWrite(name, writing)) {
+            cache.delete(name);
+        } else {
+            deleteStorageFile(name);
+        }
+        if (includeLogging) {
+            Log.e(err);
+            if (Log.isCrashBound()) {
+                Log.sendLog();
+            }
+        }
+    }
+
     /// Reads the object from the storage, returns null if the object isn't there
     ///
     /// The sample below demonstrates the usage and registration of the `com.codename1.io.Externalizable` interface:

CodenameOne/src/com/codename1/io/Util.java

@@ -714,24 +714,45 @@ public static void writeObject(Object o, DataOutputStream out) throws IOExceptio
         if (o instanceof Hashtable) {
             Hashtable v = (Hashtable) o;
             out.writeUTF("java.util.Hashtable");
-            out.writeInt(v.size());
+            // the pairs are collected before the count is written, because the count
+            // and the entries that follow it have to describe the same map. Reading
+            // the size and then walking the map let a change from another thread
+            // produce a file whose count did not match its contents, and readObject
+            // has no way to notice: it reads exactly count entries off a stream that
+            // no longer lines up, and hands back garbage.
+            Object[] keys = new Object[v.size()];
+            Object[] values = new Object[keys.length];
+            int count = 0;
             Enumeration k = v.keys();
-            while (k.hasMoreElements()) {
+            while (k.hasMoreElements() && count < keys.length) {
                 Object key = k.nextElement();
-                writeObject(key, out);
-                writeObject(v.get(key), out);
+                keys[count] = key;
+                values[count] = v.get(key);
+                count++;
             }
+            writePairs(keys, values, count, out);
             return;
         }
         if (o instanceof Map) {
             Map v = (Map) o;
             out.writeUTF("java.util.Map");
-            out.writeInt(v.size());
+            // collected up front for the reason given above the Hashtable case. The
+            // key and the value are copied out of each entry rather than the entry
+            // itself kept, since a Map is free to hand the same mutable entry object
+            // to every step of the iteration.
+            Object[] keys = new Object[v.size()];
+            Object[] values = new Object[keys.length];
+            int count = 0;
             for (Object entryObj : v.entrySet()) {
+                if (count == keys.length) {
+                    break;
+                }
                 Map.Entry entry = (Map.Entry) entryObj;
-                writeObject(entry.getKey(), out);
-                writeObject(entry.getValue(), out);
+                keys[count] = entry.getKey();
+                values[count] = entry.getValue();
+                count++;
             }
+            writePairs(keys, values, count, out);
             return;
         }
 
@@ -882,6 +903,27 @@ public static void writeObject(Object o, DataOutputStream out) throws IOExceptio
                 + " value: " + o);
     }
 
+    /// Writes a count followed by exactly that many key/value pairs, so the header
+    /// always describes the payload that follows it.
+    ///
+    /// #### Parameters
+    ///
+    /// - `keys`: the keys to write
+    ///
+    /// - `values`: the values to write, matched to `keys` by position
+    ///
+    /// - `count`: how many entries of the two arrays to write
+    ///
+    /// - `out`: the destination stream
+    private static void writePairs(Object[] keys, Object[] values, int count,
+            DataOutputStream out) throws IOException {
+        out.writeInt(count);
+        for (int iter = 0; iter < count; iter++) {
+            writeObject(keys[iter], out);
+            writeObject(values[iter], out);
+        }
+    }
+
     /// This method allows working around [issue 58](http://code.google.com/p/codenameone/issues/detail?id=58)
     ///
     /// #### Parameters