From b28377d39b416a39edb776e1a4b9883bee08049c Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:37:32 +0300 Subject: [PATCH 01/17] Stop Android storage entries coming back empty after an abrupt shutdown A storage entry on Android was written by openFileOutput, which truncates the entry as it opens it, and Android does not flush a file on close. Every write therefore had two windows in which the entry on disk was not the entry the app had stored: while the bytes were being written it was empty or half written, and once it was written it stayed in the page cache for as long as the kernel felt like holding it. An abrupt end to the process or the device inside either window -- a low memory kill, a force stop, a battery pull, a panic -- lost the entry, and on a filesystem that journals the truncation ahead of the data it came back as a zero length file. How wide those windows are is a property of the filesystem and of how eagerly the vendor kills background processes, which is why this only showed up on some devices. The port has carried an fsync for exactly this since the beginning, in closingOutput, whose comment cites Android's own note on the subject. It has never run on this path: its only caller is BufferedOutputStream.close, and the Android storage path hands back the raw FileOutputStream. iOS wraps, so it does call it. An entry is now written to a scratch file that is synced and then renamed over the entry, so the entry changes in one step no filesystem can show half done and the bytes reach the device before that step is taken. Wrapping the existing stream in a BufferedOutputStream would have restored the fsync, but it leaves the entry truncated in place and so leaves the first window open. Three more ways the same data could go missing, found while reading the path around it: Storage.writeObject cached the object before writing it and, when the write failed, deleted the entry through the implementation, which skips the cache. The stale copy then answered every read for the rest of the session, so the failure only surfaced as missing data after a restart. Util.writeObject wrote a map's entry count and then walked the map. A change arriving from another thread in between produced a file whose count did not match its contents, which readObject cannot detect -- it reads exactly count entries off a stream that no longer lines up. The pairs are now collected before the count is written, so the header always describes the payload, and the key and value are copied out of each entry rather than the entry kept, since a Map may hand out one mutable entry for the whole iteration. Preferences.set mutated the map and then called save without holding the lock save takes, which is how a real application reached the case above. It holds the lock across both now, and fires listeners outside it. Preferences is a single file rewritten in full on every set, so this took out every preference at once rather than one. Tests cover the three core fixes; each fails without it. The map case shows the damage is not confined to the object being written: the misaligned stream corrupted the next object read after it. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/com/codename1/io/Preferences.java | 77 +++--- CodenameOne/src/com/codename1/io/Storage.java | 6 +- CodenameOne/src/com/codename1/io/Util.java | 56 ++++- .../impl/android/AndroidImplementation.java | 229 +++++++++++++++++- .../java/com/codename1/io/StorageTest.java | 14 ++ .../test/java/com/codename1/io/UtilTest.java | 126 ++++++++++ .../TestCodenameOneImplementation.java | 19 +- 7 files changed, 485 insertions(+), 42 deletions(-) diff --git a/CodenameOne/src/com/codename1/io/Preferences.java b/CodenameOne/src/com/codename1/io/Preferences.java index 937c84479fe..0ea411534c9 100644 --- a/CodenameOne/src/com/codename1/io/Preferences.java +++ b/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 values) { ArrayList changeParams = new ArrayList(); - for (Map.Entry 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 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,9 +212,12 @@ 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); } @@ -212,21 +225,23 @@ public static void delete(String pref) { public static void clearAll() { // We only need to save prior values for Preferences that actually have listeners. Hashtable priorValues = null; - if (!listenerMap.isEmpty()) { - - // Save all the Preferences for which there are registered listeners. - priorValues = new Hashtable(); - 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(); + 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); diff --git a/CodenameOne/src/com/codename1/io/Storage.java b/CodenameOne/src/com/codename1/io/Storage.java index ea9dc70fcb8..fee6ec8df64 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -483,7 +483,11 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { Log.sendLog(); } } - Util.getImplementation().deleteStorageFile(name); + // the entry is gone, so the cached copy has to go with it. 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. + deleteStorageFile(name); return false; } finally { Util.getImplementation().cleanup(d); diff --git a/CodenameOne/src/com/codename1/io/Util.java b/CodenameOne/src/com/codename1/io/Util.java index 6ed3c3965de..238460f67c7 100644 --- a/CodenameOne/src/com/codename1/io/Util.java +++ b/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 diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 7ee774f6815..85a5324bb45 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7506,18 +7506,39 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti } + /** + * Marks a file that holds a storage write still in progress. Such a file is + * invisible to every storage entry point and is renamed over the real entry + * once its bytes are on the device. + */ + private static final String STORAGE_SCRATCH_SUFFIX = ".cn1tmp"; + + /** + * Distinguishes the scratch files of concurrent writes, so two threads writing + * the same entry cannot interleave their bytes into one file. + */ + private static final AtomicLong storageScratchCounter = new AtomicLong(); + + /** + * Whether the scratch files abandoned by a previous run of this application have + * been removed. Guarded by the class monitor. + */ + private static boolean storageScratchSwept; + /** * @inheritDoc */ public void deleteStorageFile(String name) { getContext().deleteFile(name); + discardStorageScratchFiles(name); } /** * @inheritDoc */ public OutputStream createStorageOutputStream(String name) throws IOException { - return getContext().openFileOutput(name, 0); + sweepStorageScratchFiles(); + return new StorageOutputStream(name); } /** @@ -7531,6 +7552,9 @@ public InputStream createStorageInputStream(String name) throws IOException { * @inheritDoc */ public boolean storageFileExists(String name) { + if (isStorageScratchName(name)) { + return false; + } String[] fileList = getContext().fileList(); for (int iter = 0; iter < fileList.length; iter++) { if (fileList[iter].equals(name)) { @@ -7544,16 +7568,217 @@ public boolean storageFileExists(String name) { * @inheritDoc */ public String[] listStorageEntries() { - return getContext().fileList(); + String[] fileList = getContext().fileList(); + int keep = 0; + for (int iter = 0; iter < fileList.length; iter++) { + if (!isStorageScratchName(fileList[iter])) { + fileList[keep] = fileList[iter]; + keep++; + } + } + if (keep == fileList.length) { + return fileList; + } + String[] entries = new String[keep]; + System.arraycopy(fileList, 0, entries, 0, keep); + return entries; } /** * @inheritDoc */ public int getStorageEntrySize(String name) { + if (isStorageScratchName(name)) { + return 0; + } return (int)new File(getContext().getFilesDir(), name).length(); } + /** + * Whether the given file holds a storage write in progress rather than a storage + * entry of its own. + * + * @param name the file name + * @return true when the file belongs to a write in progress + */ + private static boolean isStorageScratchName(String name) { + int suffix = name.lastIndexOf(STORAGE_SCRATCH_SUFFIX); + if (suffix < 0 || suffix + STORAGE_SCRATCH_SUFFIX.length() >= name.length()) { + return false; + } + // the counter that follows the suffix is what keeps an entry whose own name + // happens to end in ".cn1tmp" visible + for (int iter = suffix + STORAGE_SCRATCH_SUFFIX.length(); iter < name.length(); iter++) { + char c = name.charAt(iter); + if (c < '0' || c > '9') { + return false; + } + } + return true; + } + + /** + * Abandons any write still in progress against the given entry, so that a caller + * that deletes an entry it has just failed to write does not find those failed + * bytes renamed over it afterwards. + * + * @param name the storage entry + */ + private void discardStorageScratchFiles(String name) { + String prefix = name + STORAGE_SCRATCH_SUFFIX; + String[] fileList = getContext().fileList(); + for (int iter = 0; iter < fileList.length; iter++) { + if (fileList[iter].startsWith(prefix) && isStorageScratchName(fileList[iter])) { + getContext().deleteFile(fileList[iter]); + } + } + } + + /** + * Removes the scratch files left behind by a previous run that died mid write. + * They are already invisible to the storage API, this only stops them + * accumulating. + * + *

Runs before the first scratch file of this process exists, and holds the + * monitor across the sweep, so it cannot delete a write that is in flight.

+ */ + private void sweepStorageScratchFiles() { + synchronized (AndroidImplementation.class) { + if (storageScratchSwept) { + return; + } + storageScratchSwept = true; + try { + String[] fileList = getContext().fileList(); + for (int iter = 0; iter < fileList.length; iter++) { + if (isStorageScratchName(fileList[iter])) { + getContext().deleteFile(fileList[iter]); + } + } + } catch (Throwable t) { + // a sweep that fails costs disk space, never correctness + com.codename1.io.Log.e(t); + } + } + } + + /** + * Writes a storage entry to a scratch file, forces the bytes onto the device and + * only then renames that file over the entry. + * + *

{@code openFileOutput} truncates the entry as it opens it, and Android does + * not flush a file on close. Writing the entry in place therefore left a window + * on every single write in which the entry was empty or half written on disk, and + * left the bytes of a completed write sitting in the page cache for as long as + * the kernel felt like holding them. An abrupt end to the process or to the + * device inside either window -- a low memory kill, a force stop, a battery pull, + * a panic -- lost the entry, and on a filesystem that journals the truncation + * ahead of the data it came back as a zero length file. How wide those windows + * are is a property of the filesystem and of how eagerly the vendor kills + * background processes, which is why this only ever showed up on some devices.

+ * + *

The entry now changes in a single rename, which the filesystem cannot show + * half done, and the bytes reach the device before that rename is made.

+ */ + private final class StorageOutputStream extends OutputStream { + private final String name; + private final String scratchName; + private final FileOutputStream out; + private boolean closed; + + StorageOutputStream(String name) throws IOException { + this.name = name; + this.scratchName = name + STORAGE_SCRATCH_SUFFIX + + storageScratchCounter.incrementAndGet(); + this.out = getContext().openFileOutput(scratchName, 0); + } + + @Override + public void write(int b) throws IOException { + out.write(b); + } + + @Override + public void write(byte[] b) throws IOException { + out.write(b); + } + + @Override + public void write(byte[] b, int off, int len) throws IOException { + out.write(b, off, len); + } + + @Override + public void flush() throws IOException { + out.flush(); + } + + @Override + public void close() throws IOException { + if (closed) { + return; + } + closed = true; + try { + out.flush(); + out.getFD().sync(); + } finally { + out.close(); + } + File dir = getContext().getFilesDir(); + File scratch = new File(dir, scratchName); + if (scratch.renameTo(new File(dir, name))) { + syncStorageDirectory(dir); + return; + } + if (!scratch.exists()) { + // deleteStorageFile abandoned this write while it was open, which is + // how a caller cancels one; there is nothing left to publish + return; + } + getContext().deleteFile(scratchName); + throw new IOException("Could not store " + name); + } + } + + /** + * Forces a rename in the given directory onto the device, so that a completed + * write does not fall back to its previous contents after an abrupt shutdown. + * Best effort: without it a crash can still only cost the newest write, never the + * integrity of an entry. + * + * @param dir the directory holding the storage entries + */ + private static void syncStorageDirectory(File dir) { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) { + return; + } + try { + DirectorySync.sync(dir); + } catch (Throwable t) { + // some filesystems refuse to sync a directory handle + } + } + + /** + * Isolates the API 21 syscalls, so that verifying {@code AndroidImplementation} + * on an older device never has to resolve them. + */ + private static final class DirectorySync { + private DirectorySync() { + } + + static void sync(File dir) throws android.system.ErrnoException { + java.io.FileDescriptor fd = android.system.Os.open(dir.getPath(), + android.system.OsConstants.O_RDONLY, 0); + try { + android.system.Os.fsync(fd); + } finally { + android.system.Os.close(fd); + } + } + } + private String addFile(String s) { // I explicitly don't create a "proper URL" since code might rely on the fact that the file isn't encoded if(s != null && s.startsWith("/")) { diff --git a/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java b/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java index efc4245e58c..21cab7895ce 100644 --- a/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java @@ -108,4 +108,18 @@ void existsDelegatesToImplementationWithNormalization() { implementation.putStorageEntry("needs_normalization", new byte[]{1}); assertTrue(storage.exists(key)); } + + @EdtTest + void failedWriteLeavesNothingBehindInTheCache() { + String key = "unwritable"; + assertTrue(storage.writeObject(key, "the value that is really stored")); + + // Object is not one of the supported types, so Util.writeObject throws and + // the entry is removed. The cached copy has to go with it, otherwise reads + // keep answering from memory until the app is restarted. + assertFalse(storage.writeObject(key, new Object(), false)); + + assertFalse(storage.exists(key)); + assertNull(storage.readObject(key)); + } } diff --git a/maven/core-unittests/src/test/java/com/codename1/io/UtilTest.java b/maven/core-unittests/src/test/java/com/codename1/io/UtilTest.java index 119bf0319a9..acee9679d21 100644 --- a/maven/core-unittests/src/test/java/com/codename1/io/UtilTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/io/UtilTest.java @@ -12,9 +12,13 @@ import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; +import java.util.AbstractMap; import java.util.HashMap; +import java.util.Hashtable; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Set; import static org.junit.jupiter.api.Assertions.*; @@ -148,4 +152,126 @@ void readToStringUsesProvidedCharset() throws IOException { String value = Util.readToString(new ByteArrayInputStream(data), "UTF-16BE"); assertEquals("héllo", value); } + + @EdtTest + void mapWritesAsManyEntriesAsItsHeaderPromises() throws IOException { + // a map that reports more entries than it hands out stands in for one that + // another thread shrank between the size being read and the walk that + // follows it. The header and the payload have to agree either way, since a + // reader that trusts the header reads straight off the end of the entries. + Map shrunk = new LyingSizeMap(3); + shrunk.put("kept", "a"); + shrunk.put("alsoKept", "b"); + + Map result = roundTripMap(shrunk); + + assertEquals(2, result.size()); + assertEquals("a", result.get("kept")); + assertEquals("b", result.get("alsoKept")); + } + + @EdtTest + void mapNeverWritesMoreEntriesThanItsHeaderPromises() throws IOException { + // the other direction: a map that grew. The extra entries must not be + // written, or everything after this object in the stream reads as garbage. + Map grown = new LyingSizeMap(1); + grown.put("first", "a"); + grown.put("second", "b"); + grown.put("third", "c"); + + ByteArrayOutputStream output = new ByteArrayOutputStream(); + DataOutputStream dataOutput = new DataOutputStream(output); + Util.writeObject(grown, dataOutput); + Util.writeObject("sentinel", dataOutput); + dataOutput.close(); + + DataInputStream input = new DataInputStream(new ByteArrayInputStream(output.toByteArray())); + Object map = Util.readObject(input); + Object sentinel = Util.readObject(input); + input.close(); + + assertTrue(map instanceof Map); + assertEquals(1, ((Map) map).size()); + // the stream is still aligned for whatever was written after the map + assertEquals("sentinel", sentinel); + } + + @EdtTest + void hashtableWritesAsManyEntriesAsItsHeaderPromises() throws IOException { + Hashtable shrunk = new LyingSizeHashtable(4); + shrunk.put("one", "1"); + shrunk.put("two", "2"); + + ByteArrayOutputStream output = new ByteArrayOutputStream(); + DataOutputStream dataOutput = new DataOutputStream(output); + Util.writeObject(shrunk, dataOutput); + dataOutput.close(); + + DataInputStream input = new DataInputStream(new ByteArrayInputStream(output.toByteArray())); + Object result = Util.readObject(input); + input.close(); + + assertTrue(result instanceof Hashtable); + Hashtable roundTrip = (Hashtable) result; + assertEquals(2, roundTrip.size()); + assertEquals("1", roundTrip.get("one")); + assertEquals("2", roundTrip.get("two")); + } + + private Map roundTripMap(Map value) throws IOException { + ByteArrayOutputStream output = new ByteArrayOutputStream(); + DataOutputStream dataOutput = new DataOutputStream(output); + Util.writeObject(value, dataOutput); + dataOutput.close(); + + DataInputStream input = new DataInputStream(new ByteArrayInputStream(output.toByteArray())); + Object result = Util.readObject(input); + input.close(); + + assertTrue(result instanceof Map); + @SuppressWarnings("unchecked") + Map typed = (Map) result; + return typed; + } + + /// A Map whose reported size does not match the entries it iterates, which is + /// what a map being changed on another thread looks like from the serializer. + private static final class LyingSizeMap extends AbstractMap { + private final Map delegate = new LinkedHashMap(); + private final int reportedSize; + + LyingSizeMap(int reportedSize) { + this.reportedSize = reportedSize; + } + + @Override + public Set> entrySet() { + return delegate.entrySet(); + } + + @Override + public Object put(String key, Object value) { + return delegate.put(key, value); + } + + @Override + public int size() { + return reportedSize; + } + } + + /// The Hashtable equivalent of {@link LyingSizeMap}; Util serializes Hashtable + /// through its own branch. + private static final class LyingSizeHashtable extends Hashtable { + private final int reportedSize; + + LyingSizeHashtable(int reportedSize) { + this.reportedSize = reportedSize; + } + + @Override + public synchronized int size() { + return reportedSize; + } + } } diff --git a/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java b/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java index 09971fb4ebf..a147d08a1dc 100644 --- a/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java +++ b/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java @@ -98,6 +98,7 @@ */ public class TestCodenameOneImplementation extends CodenameOneImplementation { private final Map storageEntries = new ConcurrentHashMap<>(); + private final List openStorageWrites = new CopyOnWriteArrayList<>(); private final Map fileSystem = new ConcurrentHashMap<>(); private final Map connections = new ConcurrentHashMap<>(); private final Map sockets = new ConcurrentHashMap<>(); @@ -3101,6 +3102,11 @@ public String[] getHeaderFields(String name, Object connection) { @Override public void deleteStorageFile(String name) { storageEntries.remove(name); + // a real port publishes an entry by replacing it, so deleting one abandons + // any write still open against it rather than being undone by it + for (StorageOutput open : openStorageWrites) { + open.discard(name); + } } public void putStorageEntry(String name, byte[] data) { @@ -4359,15 +4365,26 @@ public String toString() { private final class StorageOutput extends ByteArrayOutputStream { private final String name; + private volatile boolean discarded; StorageOutput(String name) { this.name = name; + openStorageWrites.add(this); + } + + void discard(String entry) { + if (name.equals(entry)) { + discarded = true; + } } @Override public void close() throws IOException { super.close(); - storageEntries.put(name, toByteArray()); + openStorageWrites.remove(this); + if (!discarded) { + storageEntries.put(name, toByteArray()); + } } } From d480d6059c88d428a1515df0fdeb14966d3324f6 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:58:46 +0300 Subject: [PATCH 02/17] Address review: publish failures, scratch namespace, delete/publish race Storage.writeObject returned true before its finally block closed the stream. Closing is where an implementation that replaces the entry in one step does the writing, so a failed fsync or rename reached cleanup(), which logs and swallows, and the method reported a write that never landed while keeping the new object in its cache. The stream is closed on the success path now, where the failure can still change the answer. Scratch files lived beside the entries under a name suffix, and any key ending in that suffix followed by digits -- session.cn1tmp1 -- was taken for one: invisible to exists() and listEntries(), and swept away by the next process. No pattern over a flat namespace can rule that out, so they moved to a directory of their own, where nothing an application can name reaches them. Deleting an entry raced the rename that publishes one: a write already mid close could put back an entry another thread had just deleted. Unlinking the path used to make that impossible on its own, since the write was left holding a descriptor on an inode with no name. Deletion now cancels the open writes for that entry and does so under the lock the rename takes, so the two take turns and the delete stays the later word. StorageOutputStream is static; getContext() is, so it never needed the outer instance (SpotBugs SIC_INNER_SHOULD_BE_STATIC_NEEDS_THIS). Adds the GPLv2 + Classpath Exception header to the two test files that lacked one, for check-copyright-headers. Co-Authored-By: Claude Opus 5 (1M context) --- CodenameOne/src/com/codename1/io/Storage.java | 7 + .../impl/android/AndroidImplementation.java | 192 +++++++++++------- .../java/com/codename1/io/StorageTest.java | 41 ++++ .../test/java/com/codename1/io/UtilTest.java | 23 +++ .../TestCodenameOneImplementation.java | 17 +- 5 files changed, 203 insertions(+), 77 deletions(-) diff --git a/CodenameOne/src/com/codename1/io/Storage.java b/CodenameOne/src/com/codename1/io/Storage.java index fee6ec8df64..cde04be9b91 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -475,6 +475,13 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { try { d = new DataOutputStream(createOutputStream(name)); 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. + DataOutputStream writing = d; + d = null; + writing.close(); return true; } catch (Exception err) { if (includeLogging) { diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 85a5324bb45..11cffcb38d2 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7507,11 +7507,15 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti } /** - * Marks a file that holds a storage write still in progress. Such a file is - * invisible to every storage entry point and is renamed over the real entry - * once its bytes are on the device. + * Directory under the files dir that holds storage writes still in progress. + * + *

A directory rather than a suffix on the entry name: a name the storage API + * accepts must never be mistaken for a write in progress, and no pattern over a + * flat namespace can promise that. Nothing but this implementation puts anything + * in here, so a scratch file cannot collide with an entry however the entry is + * named.

*/ - private static final String STORAGE_SCRATCH_SUFFIX = ".cn1tmp"; + private static final String STORAGE_SCRATCH_DIR = ".cn1-storage-scratch"; /** * Distinguishes the scratch files of concurrent writes, so two threads writing @@ -7519,9 +7523,24 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti */ private static final AtomicLong storageScratchCounter = new AtomicLong(); + /** + * Guards the instant at which a write is published or abandoned, and the set of + * writes that are still open. Deleting an entry and publishing one have to take + * turns: otherwise a write that renames its scratch file just after another + * thread deleted the entry brings the deleted entry back. + */ + private static final Object storagePublishLock = new Object(); + + /** + * The writes that are currently open, so that deleting an entry can cancel them. + * Guarded by {@link #storagePublishLock}. + */ + private static final List openStorageWrites = + new ArrayList(); + /** * Whether the scratch files abandoned by a previous run of this application have - * been removed. Guarded by the class monitor. + * been removed. Guarded by {@link #storagePublishLock}. */ private static boolean storageScratchSwept; @@ -7529,8 +7548,16 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti * @inheritDoc */ public void deleteStorageFile(String name) { - getContext().deleteFile(name); - discardStorageScratchFiles(name); + synchronized (storagePublishLock) { + // cancelled before the entry goes, and under the same lock the publishing + // rename takes, so a write that is already mid close cannot put the entry + // back afterwards. Unlinking the entry used to make that impossible on its + // own, since the write held a descriptor on an inode with no name left. + for (int iter = 0; iter < openStorageWrites.size(); iter++) { + openStorageWrites.get(iter).cancel(name); + } + getContext().deleteFile(name); + } } /** @@ -7552,7 +7579,7 @@ public InputStream createStorageInputStream(String name) throws IOException { * @inheritDoc */ public boolean storageFileExists(String name) { - if (isStorageScratchName(name)) { + if (STORAGE_SCRATCH_DIR.equals(name)) { return false; } String[] fileList = getContext().fileList(); @@ -7571,7 +7598,7 @@ public String[] listStorageEntries() { String[] fileList = getContext().fileList(); int keep = 0; for (int iter = 0; iter < fileList.length; iter++) { - if (!isStorageScratchName(fileList[iter])) { + if (!STORAGE_SCRATCH_DIR.equals(fileList[iter])) { fileList[keep] = fileList[iter]; keep++; } @@ -7588,71 +7615,36 @@ public String[] listStorageEntries() { * @inheritDoc */ public int getStorageEntrySize(String name) { - if (isStorageScratchName(name)) { + if (STORAGE_SCRATCH_DIR.equals(name)) { return 0; } return (int)new File(getContext().getFilesDir(), name).length(); } - /** - * Whether the given file holds a storage write in progress rather than a storage - * entry of its own. - * - * @param name the file name - * @return true when the file belongs to a write in progress - */ - private static boolean isStorageScratchName(String name) { - int suffix = name.lastIndexOf(STORAGE_SCRATCH_SUFFIX); - if (suffix < 0 || suffix + STORAGE_SCRATCH_SUFFIX.length() >= name.length()) { - return false; - } - // the counter that follows the suffix is what keeps an entry whose own name - // happens to end in ".cn1tmp" visible - for (int iter = suffix + STORAGE_SCRATCH_SUFFIX.length(); iter < name.length(); iter++) { - char c = name.charAt(iter); - if (c < '0' || c > '9') { - return false; - } - } - return true; - } - - /** - * Abandons any write still in progress against the given entry, so that a caller - * that deletes an entry it has just failed to write does not find those failed - * bytes renamed over it afterwards. - * - * @param name the storage entry - */ - private void discardStorageScratchFiles(String name) { - String prefix = name + STORAGE_SCRATCH_SUFFIX; - String[] fileList = getContext().fileList(); - for (int iter = 0; iter < fileList.length; iter++) { - if (fileList[iter].startsWith(prefix) && isStorageScratchName(fileList[iter])) { - getContext().deleteFile(fileList[iter]); - } - } - } - /** * Removes the scratch files left behind by a previous run that died mid write. * They are already invisible to the storage API, this only stops them * accumulating. * - *

Runs before the first scratch file of this process exists, and holds the - * monitor across the sweep, so it cannot delete a write that is in flight.

+ *

Every write calls this before it creates its scratch file and the sweep + * itself holds the lock, so the one sweep that runs is over before any scratch + * file of this process exists and cannot delete a write that is in flight.

*/ private void sweepStorageScratchFiles() { - synchronized (AndroidImplementation.class) { + synchronized (storagePublishLock) { if (storageScratchSwept) { return; } storageScratchSwept = true; try { - String[] fileList = getContext().fileList(); - for (int iter = 0; iter < fileList.length; iter++) { - if (isStorageScratchName(fileList[iter])) { - getContext().deleteFile(fileList[iter]); + File[] abandoned = storageScratchDir().listFiles(); + if (abandoned == null) { + return; + } + for (int iter = 0; iter < abandoned.length; iter++) { + if (!abandoned[iter].delete()) { + com.codename1.io.Log.p("Could not remove the abandoned storage " + + "scratch file " + abandoned[iter]); } } } catch (Throwable t) { @@ -7662,6 +7654,15 @@ private void sweepStorageScratchFiles() { } } + /** + * The directory holding the writes that are in progress. + * + * @return the scratch directory, which is not guaranteed to exist yet + */ + private static File storageScratchDir() { + return new File(getContext().getFilesDir(), STORAGE_SCRATCH_DIR); + } + /** * Writes a storage entry to a scratch file, forces the bytes onto the device and * only then renames that file over the entry. @@ -7680,17 +7681,38 @@ private void sweepStorageScratchFiles() { *

The entry now changes in a single rename, which the filesystem cannot show * half done, and the bytes reach the device before that rename is made.

*/ - private final class StorageOutputStream extends OutputStream { + private static final class StorageOutputStream extends OutputStream { private final String name; - private final String scratchName; + private final File scratch; private final FileOutputStream out; private boolean closed; + private boolean cancelled; StorageOutputStream(String name) throws IOException { this.name = name; - this.scratchName = name + STORAGE_SCRATCH_SUFFIX - + storageScratchCounter.incrementAndGet(); - this.out = getContext().openFileOutput(scratchName, 0); + File dir = storageScratchDir(); + if (!dir.isDirectory() && !dir.mkdirs() && !dir.isDirectory()) { + throw new IOException("Could not create the storage scratch directory " + + dir); + } + this.scratch = new File(dir, name + "." + storageScratchCounter.incrementAndGet()); + this.out = new FileOutputStream(scratch); + synchronized (storagePublishLock) { + openStorageWrites.add(this); + } + } + + /** + * Marks this write as one that must not be published, because the entry it + * would publish over has been deleted since it opened. Called holding + * {@link #storagePublishLock}. + * + * @param entry the entry being deleted + */ + void cancel(String entry) { + if (name.equals(entry)) { + cancelled = true; + } } @Override @@ -7720,24 +7742,42 @@ public void close() throws IOException { } closed = true; try { - out.flush(); - out.getFD().sync(); + try { + out.flush(); + out.getFD().sync(); + } finally { + out.close(); + } + publish(); } finally { - out.close(); + synchronized (storagePublishLock) { + openStorageWrites.remove(this); + } + if (scratch.exists() && !scratch.delete()) { + com.codename1.io.Log.p("Could not remove the storage scratch file " + + scratch); + } } - File dir = getContext().getFilesDir(); - File scratch = new File(dir, scratchName); - if (scratch.renameTo(new File(dir, name))) { + } + + /** + * Renames the scratch file over the entry, which is the point at which the + * write becomes visible. + * + * @throws IOException if the entry could not be replaced, so that the caller + * that wrote it hears about it rather than being told the write succeeded + */ + private void publish() throws IOException { + synchronized (storagePublishLock) { + if (cancelled) { + return; + } + File dir = getContext().getFilesDir(); + if (!scratch.renameTo(new File(dir, name))) { + throw new IOException("Could not store " + name); + } syncStorageDirectory(dir); - return; - } - if (!scratch.exists()) { - // deleteStorageFile abandoned this write while it was open, which is - // how a caller cancels one; there is nothing left to publish - return; } - getContext().deleteFile(scratchName); - throw new IOException("Could not store " + name); } } diff --git a/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java b/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java index 21cab7895ce..5bb6953995d 100644 --- a/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java @@ -1,3 +1,26 @@ +/* + * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Codename One designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Codename One through http://www.codenameone.com/ if you + * need additional information or have any questions. + */ + package com.codename1.io; import com.codename1.junit.EdtTest; @@ -122,4 +145,22 @@ void failedWriteLeavesNothingBehindInTheCache() { assertFalse(storage.exists(key)); assertNull(storage.readObject(key)); } + + @EdtTest + void writeReportsFailureWhenTheEntryCannotBePublished() { + String key = "unpublishable"; + implementation.setStorageWriteFailsOnClose(true); + try { + // an implementation that replaces the entry in one step does the writing + // as the stream closes, so that is where it can fail. Reporting success + // for a write that never landed leaves the caller trusting a value the + // storage does not have. + assertFalse(storage.writeObject(key, "value")); + } finally { + implementation.setStorageWriteFailsOnClose(false); + } + + assertFalse(storage.exists(key)); + assertNull(storage.readObject(key)); + } } diff --git a/maven/core-unittests/src/test/java/com/codename1/io/UtilTest.java b/maven/core-unittests/src/test/java/com/codename1/io/UtilTest.java index acee9679d21..3af224abbf3 100644 --- a/maven/core-unittests/src/test/java/com/codename1/io/UtilTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/io/UtilTest.java @@ -1,3 +1,26 @@ +/* + * Copyright (c) 2026, Codename One and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Codename One designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Codename One through http://www.codenameone.com/ if you + * need additional information or have any questions. + */ + package com.codename1.io; import com.codename1.junit.EdtTest; diff --git a/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java b/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java index a147d08a1dc..0849a6d3084 100644 --- a/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java +++ b/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java @@ -99,6 +99,7 @@ public class TestCodenameOneImplementation extends CodenameOneImplementation { private final Map storageEntries = new ConcurrentHashMap<>(); private final List openStorageWrites = new CopyOnWriteArrayList<>(); + private boolean storageWriteFailsOnClose; private final Map fileSystem = new ConcurrentHashMap<>(); private final Map connections = new ConcurrentHashMap<>(); private final Map sockets = new ConcurrentHashMap<>(); @@ -3109,6 +3110,16 @@ public void deleteStorageFile(String name) { } } + /** + * Makes storage writes fail at the point an entry would be published, which is + * where an implementation that replaces the entry in one step does the writing. + * + * @param failsOnClose whether closing a storage output stream should fail + */ + public void setStorageWriteFailsOnClose(boolean failsOnClose) { + storageWriteFailsOnClose = failsOnClose; + } + public void putStorageEntry(String name, byte[] data) { if (data == null) { storageEntries.remove(name); @@ -4365,7 +4376,7 @@ public String toString() { private final class StorageOutput extends ByteArrayOutputStream { private final String name; - private volatile boolean discarded; + private boolean discarded; StorageOutput(String name) { this.name = name; @@ -4382,6 +4393,10 @@ void discard(String entry) { public void close() throws IOException { super.close(); openStorageWrites.remove(this); + if (storageWriteFailsOnClose) { + // an implementation that publishes the entry on close fails here + throw new IOException("Could not store " + name); + } if (!discarded) { storageEntries.put(name, toByteArray()); } From 731413d992c48ed7eb94978c33b9ed070cbcbd83 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:01:05 +0300 Subject: [PATCH 03/17] Register a storage write under the same lock that cancels one The scratch file was created before the write was registered, so a deleteStorageFile arriving in between found nothing to cancel and the write went on to rename itself over the entry that had just been deleted. Same shape as the race the review caught, moved rather than closed: whether a deletion can see a write is what decides it, so creating the file and becoming visible to a deletion have to be one step. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/codename1/impl/android/AndroidImplementation.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 11cffcb38d2..f3b7ef47686 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7696,8 +7696,12 @@ private static final class StorageOutputStream extends OutputStream { + dir); } this.scratch = new File(dir, name + "." + storageScratchCounter.incrementAndGet()); - this.out = new FileOutputStream(scratch); + // created and registered as one step under the lock a deletion takes. + // Registering afterwards would leave a write whose scratch file already + // exists but which a concurrent deleteStorageFile cannot see to cancel, + // and that write would rename itself over the entry that was deleted. synchronized (storagePublishLock) { + this.out = new FileOutputStream(scratch); openStorageWrites.add(this); } } From 251bcf8f6f8b50c89238f4a23f8921ff2fd5aad7 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:26:15 +0300 Subject: [PATCH 04/17] Fix the PMD CloseResource gate and park the EDT for batched AR event tests Two CI failures on the previous commit. build-test (8): the local that takes the stream over from the finally block tripped PMD's CloseResource, which is on the forbidden list. Suppressed the same way the declaration above it already is; the report is back to zero violations. build-test (17): three ARSessionTest cases assume a batch of implementation events reaches the bridge before the EDT drains it, and nothing arranged that -- the EDT is live, so it can drain between two calls and split a coalesced update into two events. That is correct behaviour for the bridge, which coalesces refinements only while they are still pending, so a fast EDT is allowed to deliver both; the tests were asserting how busy the machine was. They park the EDT while the batch is posted now. Pre-existing, and not from this branch: the same three fail about one run in ten against master's core, which is how it reached this PR looking like a new failure. Verified 0 failures in 12 runs with the fix, and the full suite green on JDK 8 and 17. Co-Authored-By: Claude Opus 5 (1M context) --- CodenameOne/src/com/codename1/io/Storage.java | 3 +- .../java/com/codename1/ar/ARSessionTest.java | 84 +++++++++++++++---- 2 files changed, 71 insertions(+), 16 deletions(-) diff --git a/CodenameOne/src/com/codename1/io/Storage.java b/CodenameOne/src/com/codename1/io/Storage.java index cde04be9b91..8c947bb7a04 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -479,7 +479,8 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { // 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. - DataOutputStream writing = d; + // handed off so the finally does not close it a second time + DataOutputStream writing = d; //NOPMD CloseResource d = null; writing.close(); return true; diff --git a/maven/core-unittests/src/test/java/com/codename1/ar/ARSessionTest.java b/maven/core-unittests/src/test/java/com/codename1/ar/ARSessionTest.java index 3d4c174136f..5af54d16fe5 100644 --- a/maven/core-unittests/src/test/java/com/codename1/ar/ARSessionTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/ar/ARSessionTest.java @@ -26,8 +26,12 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import com.codename1.ui.Display; + import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; import static org.junit.jupiter.api.Assertions.*; @@ -306,18 +310,59 @@ public void execute() { // ---- event bridge ---- + /// Runs the given work with the EDT parked, so that nothing it posts can be + /// drained before it has finished. + /// + /// The bridge coalesces per-frame refinements only for as long as they are still + /// pending, which is the right behaviour -- a fast EDT is allowed to deliver two + /// updates as two events. That makes "the same batch" something a test has to + /// arrange rather than assume: with the EDT free to run, it can drain between two + /// calls here and the coalescing under test never gets the chance to happen. + /// Parking it first is what makes these assertions about the bridge instead of + /// about how busy the machine was. + /// + /// @param batch the events to post, and any assertion about what has not been + /// delivered yet + private void inOneBatch(Runnable batch) { + final CountDownLatch parked = new CountDownLatch(1); + final CountDownLatch release = new CountDownLatch(1); + Display.getInstance().callSerially(new Runnable() { + public void run() { + parked.countDown(); + try { + release.await(10, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + }); + try { + assertTrue(parked.await(10, TimeUnit.SECONDS), "the EDT never parked"); + batch.run(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(e); + } finally { + release.countDown(); + } + flushSerialCalls(); + } + @Test void planeEventsFromBackgroundThreadArriveAfterFlush() { open(); listenAll(); - impl.onBackgroundThread(new Runnable() { + inOneBatch(new Runnable() { public void run() { - impl.sink.onPlaneAdded(plane("p1")); + impl.onBackgroundThread(new Runnable() { + public void run() { + impl.sink.onPlaneAdded(plane("p1")); + } + }); + // Nothing delivered until the EDT drains. + assertEquals(0, planeEvents.size()); } }); - // Nothing delivered until the EDT drains. - assertEquals(0, planeEvents.size()); - flushSerialCalls(); assertEquals(1, planeEvents.size()); assertEquals(ARPlaneEvent.Kind.ADDED, planeEvents.get(0).getKind()); assertEquals("p1", planeEvents.get(0).getPlane().getId()); @@ -334,13 +379,16 @@ void planeUpdatesCoalesceToTheLatestSnapshot() { final ARPlane last = new ARPlane("p1", ARPlane.Type.HORIZONTAL_UP, ARPose.IDENTITY, 5f, 5f, null, ARTrackingState.TRACKING); - impl.onBackgroundThread(new Runnable() { + inOneBatch(new Runnable() { public void run() { - impl.sink.onPlaneUpdated(plane("p1")); - impl.sink.onPlaneUpdated(last); + impl.onBackgroundThread(new Runnable() { + public void run() { + impl.sink.onPlaneUpdated(plane("p1")); + impl.sink.onPlaneUpdated(last); + } + }); } }); - flushSerialCalls(); assertEquals(1, planeEvents.size(), "two updates for one id coalesce into one event"); assertEquals(ARPlaneEvent.Kind.UPDATED, planeEvents.get(0).getKind()); assertEquals(5f, session.getPlanes()[0].getExtentX(), 0f); @@ -354,9 +402,12 @@ void planeRemovalDropsThePlaneAndSkipsStaleUpdates() { flushSerialCalls(); planeEvents.clear(); - impl.sink.onPlaneRemoved("p1"); - impl.sink.onPlaneUpdated(plane("p1")); - flushSerialCalls(); + inOneBatch(new Runnable() { + public void run() { + impl.sink.onPlaneRemoved("p1"); + impl.sink.onPlaneUpdated(plane("p1")); + } + }); assertEquals(1, planeEvents.size(), "the stale update after removal is dropped"); assertEquals(ARPlaneEvent.Kind.REMOVED, planeEvents.get(0).getKind()); assertEquals(0, session.getPlanes().length); @@ -368,9 +419,12 @@ void updateBeforeAddInTheSameBatchAppliesAfterTheAdd() { listenAll(); final ARPlane refined = new ARPlane("p1", ARPlane.Type.HORIZONTAL_UP, ARPose.IDENTITY, 3f, 3f, null, ARTrackingState.TRACKING); - impl.sink.onPlaneUpdated(refined); - impl.sink.onPlaneAdded(plane("p1")); - flushSerialCalls(); + inOneBatch(new Runnable() { + public void run() { + impl.sink.onPlaneUpdated(refined); + impl.sink.onPlaneAdded(plane("p1")); + } + }); // Ordered add applies first, then the coalesced refinement. assertEquals(2, planeEvents.size()); assertEquals(ARPlaneEvent.Kind.ADDED, planeEvents.get(0).getKind()); From fc4d9d9ff1290bcf803cc47f840f23648134eb9b Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 18:39:35 +0300 Subject: [PATCH 05/17] Address review: multi-process sweeping, clearStorage, scratch naming, Errors Five findings from the second review pass, all of them real. An application may run more than one process, each with its own copy of this class, so the sweep that removed scratch files left by an earlier run was deleting writes another process had in flight. The failed publish then sent writeObject down its error path, which deletes the entry -- a new way to lose data, in the change meant to stop losing it. There is no shared state to coordinate through and hidepid means one process cannot ask whether another is alive, so the sweep goes on age: a day, when nothing legitimate holds a storage stream open for more than moments. clearStorage is inherited and works off listStorageEntries, so a write open against an entry that does not exist yet was invisible to it, survived the clear and published afterwards. Android cancels every open write instead. The scratch directory was itself a legal storage key. An app that already had an entry by that name would find the directory could not be created and every write failing from then on, and on a fresh install that key could no longer be stored at all. No name reserved inside a namespace where every name is legal can be kept clear of the application, so the directory moved out of the files dir to a sibling, where there is nothing to collide with. The scratch file was named after the entry, and an entry name is allowed to reach the filesystem's limit by itself, so appending anything to a long key -- a URL used as a cache key gets close -- pushed it past NAME_MAX and broke a write that used to work. It is named after the writer now, process id and counter, which is a fixed size. Nothing needs the entry name on disk since cancellation became explicit state. writeObject caught Exception, so an OutOfMemoryError partway through left the stream to the finally, which closes it, and closing is now what publishes -- a few bytes of header replacing a good entry. It catches Error too, abandons the write, and rethrows. Co-Authored-By: Claude Opus 5 (1M context) --- CodenameOne/src/com/codename1/io/Storage.java | 46 ++++++-- .../impl/android/AndroidImplementation.java | 111 +++++++++++------- 2 files changed, 104 insertions(+), 53 deletions(-) diff --git a/CodenameOne/src/com/codename1/io/Storage.java b/CodenameOne/src/com/codename1/io/Storage.java index 8c947bb7a04..2d84c172b16 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -484,24 +484,48 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { d = null; writing.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, err, includeLogging); + throw err; } catch (Exception err) { - if (includeLogging) { - Log.e(err); - if (Log.isCrashBound()) { - Log.sendLog(); - } - } - // the entry is gone, so the cached copy has to go with it. 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. - deleteStorageFile(name); + failedWrite(name, err, includeLogging); return false; } finally { Util.getImplementation().cleanup(d); } } + /// 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 + /// + /// - `err`: what went wrong + /// + /// - `includeLogging`: whether the failure may be logged, which is unsafe during + /// app initialization + private void failedWrite(String name, Throwable err, boolean includeLogging) { + if (includeLogging) { + Log.e(err); + if (Log.isCrashBound()) { + Log.sendLog(); + } + } + // the entry is gone, so the cached copy has to go with it. 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. This also abandons the write + // that is still open, so nothing partial is published when it is closed. + deleteStorageFile(name); + } + /// 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: diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index f3b7ef47686..c1cc5e4b72b 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7507,19 +7507,33 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti } /** - * Directory under the files dir that holds storage writes still in progress. + * Directory holding storage writes still in progress. * - *

A directory rather than a suffix on the entry name: a name the storage API - * accepts must never be mistaken for a write in progress, and no pattern over a - * flat namespace can promise that. Nothing but this implementation puts anything - * in here, so a scratch file cannot collide with an entry however the entry is - * named.

+ *

A sibling of the files dir rather than something inside it. Every name is a + * legal storage key, so no name reserved inside that namespace can be kept clear + * of the application: a key called after the scratch area would either be + * unstorable or, if it already existed as a file, would stop the directory being + * created and fail every write from then on. Outside the namespace there is + * nothing to collide with. It stays on the same filesystem as the entries, which + * is what lets a write be published by renaming.

*/ - private static final String STORAGE_SCRATCH_DIR = ".cn1-storage-scratch"; + private static final String STORAGE_SCRATCH_DIR = "cn1-storage-scratch"; /** - * Distinguishes the scratch files of concurrent writes, so two threads writing - * the same entry cannot interleave their bytes into one file. + * How old a scratch file must be before it is taken for abandoned. + * + *

Age rather than bookkeeping because an application may run more than one + * process, each with its own copy of this class and so its own idea of what is + * open. A process that swept on behalf of all of them would delete writes another + * process had in flight, and since the failed publish makes writeObject take its + * error path, that would destroy the entry being written. Nothing legitimate + * holds a storage stream open for a day.

+ */ + private static final long STORAGE_SCRATCH_MAX_AGE = 24L * 60L * 60L * 1000L; + + /** + * Distinguishes the scratch files of concurrent writes. Paired with the process + * id, since a second process counts from the beginning as well. */ private static final AtomicLong storageScratchCounter = new AtomicLong(); @@ -7539,8 +7553,8 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti new ArrayList(); /** - * Whether the scratch files abandoned by a previous run of this application have - * been removed. Guarded by {@link #storagePublishLock}. + * Whether the scratch files abandoned by an earlier run have been removed. + * Guarded by {@link #storagePublishLock}. */ private static boolean storageScratchSwept; @@ -7560,6 +7574,22 @@ public void deleteStorageFile(String name) { } } + /** + * @inheritDoc + */ + public void clearStorage() { + synchronized (storagePublishLock) { + // every open write, not just the ones for entries that exist. A write to + // an entry that is not there yet is absent from listStorageEntries, so the + // inherited implementation never reaches it, and it would publish a new + // entry moments after the storage was supposedly emptied. + for (int iter = 0; iter < openStorageWrites.size(); iter++) { + openStorageWrites.get(iter).cancel(); + } + super.clearStorage(); + } + } + /** * @inheritDoc */ @@ -7579,9 +7609,6 @@ public InputStream createStorageInputStream(String name) throws IOException { * @inheritDoc */ public boolean storageFileExists(String name) { - if (STORAGE_SCRATCH_DIR.equals(name)) { - return false; - } String[] fileList = getContext().fileList(); for (int iter = 0; iter < fileList.length; iter++) { if (fileList[iter].equals(name)) { @@ -7595,40 +7622,19 @@ public boolean storageFileExists(String name) { * @inheritDoc */ public String[] listStorageEntries() { - String[] fileList = getContext().fileList(); - int keep = 0; - for (int iter = 0; iter < fileList.length; iter++) { - if (!STORAGE_SCRATCH_DIR.equals(fileList[iter])) { - fileList[keep] = fileList[iter]; - keep++; - } - } - if (keep == fileList.length) { - return fileList; - } - String[] entries = new String[keep]; - System.arraycopy(fileList, 0, entries, 0, keep); - return entries; + return getContext().fileList(); } /** * @inheritDoc */ public int getStorageEntrySize(String name) { - if (STORAGE_SCRATCH_DIR.equals(name)) { - return 0; - } return (int)new File(getContext().getFilesDir(), name).length(); } /** - * Removes the scratch files left behind by a previous run that died mid write. - * They are already invisible to the storage API, this only stops them - * accumulating. - * - *

Every write calls this before it creates its scratch file and the sweep - * itself holds the lock, so the one sweep that runs is over before any scratch - * file of this process exists and cannot delete a write that is in flight.

+ * Removes the scratch files left behind by a run that died mid write, once they + * are old enough that nothing can still be writing them. */ private void sweepStorageScratchFiles() { synchronized (storagePublishLock) { @@ -7641,8 +7647,9 @@ private void sweepStorageScratchFiles() { if (abandoned == null) { return; } + long oldest = System.currentTimeMillis() - STORAGE_SCRATCH_MAX_AGE; for (int iter = 0; iter < abandoned.length; iter++) { - if (!abandoned[iter].delete()) { + if (abandoned[iter].lastModified() < oldest && !abandoned[iter].delete()) { com.codename1.io.Log.p("Could not remove the abandoned storage " + "scratch file " + abandoned[iter]); } @@ -7658,9 +7665,15 @@ private void sweepStorageScratchFiles() { * The directory holding the writes that are in progress. * * @return the scratch directory, which is not guaranteed to exist yet + * @throws IOException if the application has no data directory to put it in */ - private static File storageScratchDir() { - return new File(getContext().getFilesDir(), STORAGE_SCRATCH_DIR); + private static File storageScratchDir() throws IOException { + File files = getContext().getFilesDir(); + File data = files.getParentFile(); + if (data == null) { + throw new IOException("No application data directory above " + files); + } + return new File(data, STORAGE_SCRATCH_DIR); } /** @@ -7695,7 +7708,13 @@ private static final class StorageOutputStream extends OutputStream { throw new IOException("Could not create the storage scratch directory " + dir); } - this.scratch = new File(dir, name + "." + storageScratchCounter.incrementAndGet()); + // named for the writer rather than the entry: an entry name is allowed to + // reach the filesystem's limit on its own, and anything appended to it + // would push the scratch file past that limit and fail a write that used + // to work. The process id separates concurrent processes, whose counters + // both start from the beginning. + this.scratch = new File(dir, android.os.Process.myPid() + "-" + + storageScratchCounter.incrementAndGet()); // created and registered as one step under the lock a deletion takes. // Registering afterwards would leave a write whose scratch file already // exists but which a concurrent deleteStorageFile cannot see to cancel, @@ -7706,6 +7725,14 @@ private static final class StorageOutputStream extends OutputStream { } } + /** + * Marks this write as one that must not be published, whatever entry it is + * for. Called holding {@link #storagePublishLock}. + */ + void cancel() { + cancelled = true; + } + /** * Marks this write as one that must not be published, because the entry it * would publish over has been deleted since it opened. Called holding From a3c5906ebfb24d9d2964684cf66138a3a1859c82 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:47:04 +0300 Subject: [PATCH 06/17] Address review: path escape, cross-process deletion, and sweep expiry openFileOutput refused any entry name holding a path separator, and publishing by rename does not. With name normalization turned off a key like ../shared_prefs/settings.xml reached the rename as written and File resolved it, so the write landed elsewhere in the application's private data and left an entry Storage itself could no longer read or delete. The name is resolved and checked once, when the stream opens. Cancelling a write was still process-local, so a component under its own android:process could delete an entry while another process had a write open on it and get the entry back a moment later. The fix is the property the in-place write used to have for free: deleting an entry now unlinks the scratch files being written for it, whatever process owns them, which leaves that writer holding a good descriptor on an inode with no name and nothing for its rename to find -- the same outcome deleting the open entry used to produce. Scratch files go first so a publish that slips between the two still leaves an entry for the delete to remove. Naming them after a digest of the entry is what makes them findable while staying a fixed width, which the filesystem's limit on names requires. The sweep set a flag once, so a scratch file that was merely too young when a process first wrote was never looked at again for the life of that process, however large it was. It records when the youngest file it kept comes of age instead. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/android/AndroidImplementation.java | 173 +++++++++++++++--- 1 file changed, 148 insertions(+), 25 deletions(-) diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index c1cc5e4b72b..1213bdbb1de 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7553,10 +7553,12 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti new ArrayList(); /** - * Whether the scratch files abandoned by an earlier run have been removed. - * Guarded by {@link #storagePublishLock}. + * When the scratch area is next worth looking at, as a wall clock time. Set to + * the expiry of the youngest file that was kept, so that a process which lives + * for weeks still collects a file that was merely too young the first time it + * looked. Guarded by {@link #storagePublishLock}. */ - private static boolean storageScratchSwept; + private static long nextStorageScratchSweep; /** * @inheritDoc @@ -7565,15 +7567,46 @@ public void deleteStorageFile(String name) { synchronized (storagePublishLock) { // cancelled before the entry goes, and under the same lock the publishing // rename takes, so a write that is already mid close cannot put the entry - // back afterwards. Unlinking the entry used to make that impossible on its - // own, since the write held a descriptor on an inode with no name left. + // back afterwards. for (int iter = 0; iter < openStorageWrites.size(); iter++) { openStorageWrites.get(iter).cancel(name); } + // the same for writes in another process, which this lock knows nothing + // about. Unlinking a scratch file cancels it: the writer keeps a working + // descriptor on an inode with no name, exactly as it used to keep one on + // an entry that had been deleted underneath it, and the rename that would + // have published it can no longer find anything to rename. Scratch files + // go first, so a publish that slips through between the two still leaves + // an entry for the delete below to remove. + discardScratchFilesFor(name); getContext().deleteFile(name); } } + /** + * Unlinks every scratch file being written for the given entry, in this process + * or any other, which is what cancels those writes. + * + * @param name the storage entry + */ + private static void discardScratchFilesFor(String name) { + try { + String prefix = storageScratchPrefix(name); + File[] scratch = storageScratchDir().listFiles(); + if (scratch == null) { + return; + } + for (int iter = 0; iter < scratch.length; iter++) { + if (scratch[iter].getName().startsWith(prefix) && !scratch[iter].delete()) { + com.codename1.io.Log.p("Could not cancel the storage write " + + scratch[iter]); + } + } + } catch (IOException err) { + com.codename1.io.Log.e(err); + } + } + /** * @inheritDoc */ @@ -7586,6 +7619,7 @@ public void clearStorage() { for (int iter = 0; iter < openStorageWrites.size(); iter++) { openStorageWrites.get(iter).cancel(); } + discardAllScratchFiles(); super.clearStorage(); } } @@ -7638,29 +7672,111 @@ public int getStorageEntrySize(String name) { */ private void sweepStorageScratchFiles() { synchronized (storagePublishLock) { - if (storageScratchSwept) { + long now = System.currentTimeMillis(); + if (now < nextStorageScratchSweep) { return; } - storageScratchSwept = true; + // looked at again when the youngest file that survived this pass comes of + // age. Setting a flag once instead would let a file that was a few minutes + // old at the first write of a long lived process sit there for the life of + // that process, however large it is. + long nextSweep = now + STORAGE_SCRATCH_MAX_AGE; try { File[] abandoned = storageScratchDir().listFiles(); - if (abandoned == null) { - return; - } - long oldest = System.currentTimeMillis() - STORAGE_SCRATCH_MAX_AGE; - for (int iter = 0; iter < abandoned.length; iter++) { - if (abandoned[iter].lastModified() < oldest && !abandoned[iter].delete()) { - com.codename1.io.Log.p("Could not remove the abandoned storage " - + "scratch file " + abandoned[iter]); + if (abandoned != null) { + for (int iter = 0; iter < abandoned.length; iter++) { + long expires = abandoned[iter].lastModified() + STORAGE_SCRATCH_MAX_AGE; + if (expires > now) { + nextSweep = Math.min(nextSweep, expires); + } else if (!abandoned[iter].delete()) { + com.codename1.io.Log.p("Could not remove the abandoned storage " + + "scratch file " + abandoned[iter]); + } } } } catch (Throwable t) { // a sweep that fails costs disk space, never correctness com.codename1.io.Log.e(t); } + nextStorageScratchSweep = nextSweep; } } + /** + * Unlinks every scratch file there is, cancelling every write in progress in any + * process. + */ + private static void discardAllScratchFiles() { + try { + File[] scratch = storageScratchDir().listFiles(); + if (scratch == null) { + return; + } + for (int iter = 0; iter < scratch.length; iter++) { + if (!scratch[iter].delete()) { + com.codename1.io.Log.p("Could not cancel the storage write " + + scratch[iter]); + } + } + } catch (IOException err) { + com.codename1.io.Log.e(err); + } + } + + /** + * The start of the name of every scratch file for the given entry. + * + *

A digest rather than the entry itself: an entry name may be as long as the + * filesystem allows on its own, so anything built by appending to one would be + * refused. Fixed width, and specific enough that one entry's deletion does not + * cancel another's write.

+ * + * @param name the storage entry + * @return the prefix shared by that entry's scratch files + * @throws IOException if the digest is unavailable + */ + private static String storageScratchPrefix(String name) throws IOException { + try { + byte[] digest = java.security.MessageDigest.getInstance("SHA-256") + .digest(name.getBytes("UTF-8")); + StringBuilder b = new StringBuilder(digest.length * 2); + for (int iter = 0; iter < digest.length; iter++) { + b.append(Character.forDigit((digest[iter] >> 4) & 0xf, 16)); + b.append(Character.forDigit(digest[iter] & 0xf, 16)); + } + return b.append('-').toString(); + } catch (java.security.NoSuchAlgorithmException err) { + throw new IOException("No SHA-256 to name storage scratch files with", err); + } + } + + /** + * Resolves a storage entry to its file, refusing anything that would land outside + * the storage directory. + * + *

{@code openFileOutput} used to make this check on our behalf and reject any + * name holding a path separator. Publishing by rename does not: with name + * normalization turned off a key like {@code ../shared_prefs/settings.xml} + * reaches here as it was written, and {@code File} resolves it, which would put + * the rename anywhere in the application's private data and leave behind an entry + * that Storage itself could no longer read or delete.

+ * + * @param name the storage entry + * @return the file the entry is stored in + * @throws IOException if the name does not name an entry in the storage directory + */ + private static File storageEntryFile(String name) throws IOException { + File dir = getContext().getFilesDir(); + if (name.indexOf('/') >= 0 || name.indexOf(File.separatorChar) >= 0) { + throw new IOException("Storage entry " + name + " contains a path separator"); + } + File entry = new File(dir, name); + if (!dir.equals(entry.getParentFile())) { + throw new IOException("Storage entry " + name + " resolves outside " + dir); + } + return entry; + } + /** * The directory holding the writes that are in progress. * @@ -7696,6 +7812,7 @@ private static File storageScratchDir() throws IOException { */ private static final class StorageOutputStream extends OutputStream { private final String name; + private final File target; private final File scratch; private final FileOutputStream out; private boolean closed; @@ -7703,17 +7820,17 @@ private static final class StorageOutputStream extends OutputStream { StorageOutputStream(String name) throws IOException { this.name = name; + this.target = storageEntryFile(name); File dir = storageScratchDir(); if (!dir.isDirectory() && !dir.mkdirs() && !dir.isDirectory()) { throw new IOException("Could not create the storage scratch directory " + dir); } - // named for the writer rather than the entry: an entry name is allowed to - // reach the filesystem's limit on its own, and anything appended to it - // would push the scratch file past that limit and fail a write that used - // to work. The process id separates concurrent processes, whose counters - // both start from the beginning. - this.scratch = new File(dir, android.os.Process.myPid() + "-" + // the digest of the entry lets another process find and cancel this write. + // The process id separates concurrent processes, whose counters both start + // from the beginning, and the counter separates writes within one. + this.scratch = new File(dir, storageScratchPrefix(name) + + android.os.Process.myPid() + "-" + storageScratchCounter.incrementAndGet()); // created and registered as one step under the lock a deletion takes. // Registering afterwards would leave a write whose scratch file already @@ -7803,11 +7920,17 @@ private void publish() throws IOException { if (cancelled) { return; } - File dir = getContext().getFilesDir(); - if (!scratch.renameTo(new File(dir, name))) { - throw new IOException("Could not store " + name); + if (scratch.renameTo(target)) { + syncStorageDirectory(target.getParentFile()); + return; + } + if (!scratch.exists()) { + // another process deleted this entry, or cleared the storage, + // while the write was open. Unlinking the scratch file is how it + // says so, and there is nothing left to publish. + return; } - syncStorageDirectory(dir); + throw new IOException("Could not store " + name); } } } From 41a1dbc9d19a8583835ddecba1e2aacf07e92b3c Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:55:34 +0300 Subject: [PATCH 07/17] Address review: abandon before logging, and lock creation across processes Two findings, both real. failedWrite logged before it abandoned the write. Reporting an OutOfMemoryError means building a message and a stack trace, so a second failure there carried off the rest of the method, and the write left open was published by the finally that closes it -- the partial object landing on top of the good entry, which is the case that reordering was meant to prevent in the first place. The entry and the cached copy go first now, and the logging happens after. Cancelling a write across processes worked by unlinking its scratch file, which only reaches the writes that exist when the deletion looks for them. A second process could create its scratch file just after that scan and publish over the entry the deletion went on to remove; clearStorage had the same gap. Creating a scratch file, deleting an entry and publishing a write now all run under a lock the filesystem arbitrates, so they cannot interleave between processes. The system drops that lock when a process ends however it ends, so a crash cannot leave it held, and failing to take it does not fail the write -- a storage that stops writing would be worse than one exposed to a race only a multi-process app can reach. The lock is claimed under the existing monitor and counts its nesting, since a FileLock belongs to the whole VM and cannot be taken twice, and clearStorage claims it and then deletes every entry. Co-Authored-By: Claude Opus 5 (1M context) --- CodenameOne/src/com/codename1/io/Storage.java | 16 +- .../impl/android/AndroidImplementation.java | 176 ++++++++++++++---- 2 files changed, 153 insertions(+), 39 deletions(-) diff --git a/CodenameOne/src/com/codename1/io/Storage.java b/CodenameOne/src/com/codename1/io/Storage.java index 2d84c172b16..f738b1e377d 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -512,18 +512,22 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { /// - `includeLogging`: whether the failure may be logged, which is unsafe during /// app initialization private void failedWrite(String name, 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 entry goes, and the cached copy with it. Leaving that 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. + deleteStorageFile(name); if (includeLogging) { Log.e(err); if (Log.isCrashBound()) { Log.sendLog(); } } - // the entry is gone, so the cached copy has to go with it. 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. This also abandons the write - // that is still open, so nothing partial is published when it is closed. - deleteStorageFile(name); } /// Reads the object from the storage, returns null if the object isn't there diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 1213bdbb1de..4eca978bb8b 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -192,6 +192,7 @@ import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.io.RandomAccessFile; +import java.nio.channels.FileLock; import java.io.Writer; import java.lang.reflect.Constructor; import java.net.HttpURLConnection; @@ -7545,6 +7546,95 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti */ private static final Object storagePublishLock = new Object(); + /** + * Name of the file whose lock serializes storage writes between processes. + */ + private static final String STORAGE_LOCK_FILE = ".lock"; + + /** + * The cross process lock, and the handle it is taken on, while this process holds + * it. Guarded by {@link #storagePublishLock}, so only one thread here ever has it. + */ + private static RandomAccessFile storageLockHandle; + private static FileLock storageLockAcrossProcesses; + + /** + * How many nested claims this process has on the cross process lock. A + * {@code FileLock} is held by the whole VM and cannot be taken twice, and + * clearStorage claims it and then calls deleteStorageFile for every entry. + */ + private static int storageLockDepth; + + /** + * Claims the storage for this process, so that creating a scratch file, deleting + * an entry and publishing a write cannot interleave between processes. + * + *

Unlinking a writer's scratch file is what cancels it, and that only reaches + * the writes that exist when the deletion looks. Without this a second process + * could create its scratch file just after a deletion had scanned for them, and + * publish over the entry that deletion went on to remove. A lock the filesystem + * arbitrates is the only thing both processes can see; the system drops it when a + * process ends however it ends, so it cannot be left held by a crash.

+ * + *

Best effort: if the lock cannot be taken the work still goes ahead, since a + * storage that stops writing would be worse than one exposed to a race that only + * an application with more than one process can reach at all.

+ * + *

The caller must hold {@link #storagePublishLock}.

+ */ + private static void lockStorageAcrossProcesses() { + if (storageLockDepth == 0) { + try { + File dir = storageScratchDir(); + if (dir.isDirectory() || dir.mkdirs() || dir.isDirectory()) { + RandomAccessFile handle = + new RandomAccessFile(new File(dir, STORAGE_LOCK_FILE), "rw"); + storageLockAcrossProcesses = handle.getChannel().lock(); + storageLockHandle = handle; + } + } catch (Throwable t) { + com.codename1.io.Log.e(t); + releaseStorageLock(); + } + } + storageLockDepth++; + } + + /** + * Gives up this process's claim on the storage. + * + *

The caller must hold {@link #storagePublishLock}.

+ */ + private static void unlockStorageAcrossProcesses() { + storageLockDepth--; + if (storageLockDepth == 0) { + releaseStorageLock(); + } + } + + /** + * Drops the cross process lock and the handle it was taken on, whichever of them + * this process actually got. + */ + private static void releaseStorageLock() { + try { + if (storageLockAcrossProcesses != null) { + storageLockAcrossProcesses.release(); + } + } catch (Throwable t) { + com.codename1.io.Log.e(t); + } + storageLockAcrossProcesses = null; + try { + if (storageLockHandle != null) { + storageLockHandle.close(); + } + } catch (Throwable t) { + com.codename1.io.Log.e(t); + } + storageLockHandle = null; + } + /** * The writes that are currently open, so that deleting an entry can cancel them. * Guarded by {@link #storagePublishLock}. @@ -7565,21 +7655,26 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti */ public void deleteStorageFile(String name) { synchronized (storagePublishLock) { - // cancelled before the entry goes, and under the same lock the publishing - // rename takes, so a write that is already mid close cannot put the entry - // back afterwards. - for (int iter = 0; iter < openStorageWrites.size(); iter++) { - openStorageWrites.get(iter).cancel(name); + lockStorageAcrossProcesses(); + try { + // cancelled before the entry goes, and under the same lock the + // publishing rename takes, so a write that is already mid close + // cannot put the entry back afterwards. + for (int iter = 0; iter < openStorageWrites.size(); iter++) { + openStorageWrites.get(iter).cancel(name); + } + // the same for writes in another process, which the monitor above + // knows nothing about. Unlinking a scratch file cancels it: the + // writer keeps a working descriptor on an inode with no name, exactly + // as it used to keep one on an entry deleted underneath it, and the + // rename that would have published it can no longer find anything to + // rename. Scratch files go first, so a publish that slips through + // between the two still leaves an entry for the delete to remove. + discardScratchFilesFor(name); + getContext().deleteFile(name); + } finally { + unlockStorageAcrossProcesses(); } - // the same for writes in another process, which this lock knows nothing - // about. Unlinking a scratch file cancels it: the writer keeps a working - // descriptor on an inode with no name, exactly as it used to keep one on - // an entry that had been deleted underneath it, and the rename that would - // have published it can no longer find anything to rename. Scratch files - // go first, so a publish that slips through between the two still leaves - // an entry for the delete below to remove. - discardScratchFilesFor(name); - getContext().deleteFile(name); } } @@ -7616,11 +7711,16 @@ public void clearStorage() { // an entry that is not there yet is absent from listStorageEntries, so the // inherited implementation never reaches it, and it would publish a new // entry moments after the storage was supposedly emptied. - for (int iter = 0; iter < openStorageWrites.size(); iter++) { - openStorageWrites.get(iter).cancel(); + lockStorageAcrossProcesses(); + try { + for (int iter = 0; iter < openStorageWrites.size(); iter++) { + openStorageWrites.get(iter).cancel(); + } + discardAllScratchFiles(); + super.clearStorage(); + } finally { + unlockStorageAcrossProcesses(); } - discardAllScratchFiles(); - super.clearStorage(); } } @@ -7837,8 +7937,13 @@ private static final class StorageOutputStream extends OutputStream { // exists but which a concurrent deleteStorageFile cannot see to cancel, // and that write would rename itself over the entry that was deleted. synchronized (storagePublishLock) { - this.out = new FileOutputStream(scratch); - openStorageWrites.add(this); + lockStorageAcrossProcesses(); + try { + this.out = new FileOutputStream(scratch); + openStorageWrites.add(this); + } finally { + unlockStorageAcrossProcesses(); + } } } @@ -7917,20 +8022,25 @@ public void close() throws IOException { */ private void publish() throws IOException { synchronized (storagePublishLock) { - if (cancelled) { - return; - } - if (scratch.renameTo(target)) { - syncStorageDirectory(target.getParentFile()); - return; - } - if (!scratch.exists()) { - // another process deleted this entry, or cleared the storage, - // while the write was open. Unlinking the scratch file is how it - // says so, and there is nothing left to publish. - return; + lockStorageAcrossProcesses(); + try { + if (cancelled) { + return; + } + if (scratch.renameTo(target)) { + syncStorageDirectory(target.getParentFile()); + return; + } + if (!scratch.exists()) { + // another process deleted this entry, or cleared the storage, + // while the write was open. Unlinking the scratch file is how + // it says so, and there is nothing left to publish. + return; + } + throw new IOException("Could not store " + name); + } finally { + unlockStorageAcrossProcesses(); } - throw new IOException("Could not store " + name); } } } From 0d2a3560246793b668477ae7b77f61fb9f9bfcfb Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 22:01:40 +0300 Subject: [PATCH 08/17] Keep the lock file out of the scratch cleanup loops clearStorage deleted every file in the scratch directory, the lock among them, while holding that very lock; and the sweep would have aged it out after a day, since nothing ever writes to it. Linux allows a locked file to be unlinked and the lock belongs to the inode rather than the name, so either one would let the next process create the name afresh and take a lock on a different inode. Both processes would then hold "the" lock and neither would wait for the other, which is the whole of what it was there to do. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/android/AndroidImplementation.java | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 4eca978bb8b..0146b25f2fa 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7785,6 +7785,9 @@ private void sweepStorageScratchFiles() { File[] abandoned = storageScratchDir().listFiles(); if (abandoned != null) { for (int iter = 0; iter < abandoned.length; iter++) { + if (isStorageLockFile(abandoned[iter])) { + continue; + } long expires = abandoned[iter].lastModified() + STORAGE_SCRATCH_MAX_AGE; if (expires > now) { nextSweep = Math.min(nextSweep, expires); @@ -7813,7 +7816,7 @@ private static void discardAllScratchFiles() { return; } for (int iter = 0; iter < scratch.length; iter++) { - if (!scratch[iter].delete()) { + if (!isStorageLockFile(scratch[iter]) && !scratch[iter].delete()) { com.codename1.io.Log.p("Could not cancel the storage write " + scratch[iter]); } @@ -7823,6 +7826,24 @@ private static void discardAllScratchFiles() { } } + /** + * Whether the given file is the one whose lock serializes the processes, rather + * than a write in progress. + * + *

It has to survive both the clear and the sweep. Linux lets a locked file be + * unlinked, and the lock goes with the inode rather than the name, so a process + * that removed it while holding it would leave the next process free to create + * the name afresh and take a lock on a different inode: both would then hold + * "the" lock and neither would wait for the other. Nothing writes to it either, + * so its age says nothing about whether it is in use.

+ * + * @param file a file in the scratch directory + * @return true if the file is the lock + */ + private static boolean isStorageLockFile(File file) { + return STORAGE_LOCK_FILE.equals(file.getName()); + } + /** * The start of the name of every scratch file for the given entry. * From fd1ef9443b6f18b214433407fa55a49946e1bdd5 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 22:52:04 +0300 Subject: [PATCH 09/17] Never call a vanished scratch file a successful write, and keep the handle A storage stream that lost its scratch file was treated as cancelled and closed quietly, so writeObject reported success for a value the storage never took. Deletion from another process is the case that was meant for, and failing instead reaches the same end -- writeObject deletes the entry when a write fails -- while telling the caller the truth. Everything else that could remove the file now gets the same honest answer rather than the silent loss this whole change exists to stop. Cancelling within the process stays quiet, because there the outcome is already known: the caller either asked for the entry to go or is abandoning the write itself. The sweep also skips the writes this process has open. Age cannot tell them apart on its own, since lastModified is a wall clock reading and a clock that jumps forward makes a file being written this moment look like a day old; what this process is doing it knows exactly. The lock handle is kept before the lock is attempted rather than after it succeeds, so a lock that throws still leaves something to close. A filesystem that refuses to lock was leaking a descriptor per storage operation until unrelated files stopped opening. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/android/AndroidImplementation.java | 52 +++++++++++++++---- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 0146b25f2fa..3be1a446258 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7587,10 +7587,14 @@ private static void lockStorageAcrossProcesses() { try { File dir = storageScratchDir(); if (dir.isDirectory() || dir.mkdirs() || dir.isDirectory()) { - RandomAccessFile handle = + // kept before the lock is attempted rather than after it succeeds, + // so that a lock which throws still leaves releaseStorageLock + // something to close. Otherwise a filesystem that refuses to lock + // leaks a descriptor on every storage operation until unrelated + // files stop opening. + storageLockHandle = new RandomAccessFile(new File(dir, STORAGE_LOCK_FILE), "rw"); - storageLockAcrossProcesses = handle.getChannel().lock(); - storageLockHandle = handle; + storageLockAcrossProcesses = storageLockHandle.getChannel().lock(); } } catch (Throwable t) { com.codename1.io.Log.e(t); @@ -7785,7 +7789,8 @@ private void sweepStorageScratchFiles() { File[] abandoned = storageScratchDir().listFiles(); if (abandoned != null) { for (int iter = 0; iter < abandoned.length; iter++) { - if (isStorageLockFile(abandoned[iter])) { + if (isStorageLockFile(abandoned[iter]) + || isOpenStorageWrite(abandoned[iter])) { continue; } long expires = abandoned[iter].lastModified() + STORAGE_SCRATCH_MAX_AGE; @@ -7844,6 +7849,28 @@ private static boolean isStorageLockFile(File file) { return STORAGE_LOCK_FILE.equals(file.getName()); } + /** + * Whether the given scratch file belongs to a write this process has open. + * + *

Age alone would not tell: {@code lastModified} is a wall clock reading, and + * a clock that jumps forward -- an automatic correction, say -- can make a file + * being written this moment look like a day old. What this process is doing it + * knows exactly, so it never has to guess.

+ * + *

The caller must hold {@link #storagePublishLock}.

+ * + * @param file a file in the scratch directory + * @return true if a write in this process is using it + */ + private static boolean isOpenStorageWrite(File file) { + for (int iter = 0; iter < openStorageWrites.size(); iter++) { + if (openStorageWrites.get(iter).scratch.equals(file)) { + return true; + } + } + return false; + } + /** * The start of the name of every scratch file for the given entry. * @@ -8045,6 +8072,10 @@ private void publish() throws IOException { synchronized (storagePublishLock) { lockStorageAcrossProcesses(); try { + // the one case where not publishing is not a failure: this + // process cancelled the write itself, so the caller either asked + // for the entry to go or is already abandoning the write. Failing + // here would only log noise over an outcome that is already known. if (cancelled) { return; } @@ -8052,12 +8083,13 @@ private void publish() throws IOException { syncStorageDirectory(target.getParentFile()); return; } - if (!scratch.exists()) { - // another process deleted this entry, or cleared the storage, - // while the write was open. Unlinking the scratch file is how - // it says so, and there is nothing left to publish. - return; - } + // A missing scratch file is not reported as a success. Another + // process unlinking it does mean this entry was deleted, and + // failing here reaches the same place -- writeObject deletes the + // entry on a failed write -- while still telling the caller that + // what it wrote did not land. Anything else that removed the file + // gets the same honest answer, where calling it a success would + // leave the caller believing in a value the storage never took. throw new IOException("Could not store " + name); } finally { unlockStorageAcrossProcesses(); From e703d23d069700bcaae1fe1726085fb3f8776504 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:06:18 +0300 Subject: [PATCH 10/17] A failed write must not delete the value that was already stored Both of the outstanding findings come from the same place: writeObject deleted the entry whenever a write failed. That was right when the write went into the entry, since a failure left half an object there and deleting was the only way to be rid of it. It is wrong now that the value is assembled elsewhere and put in place in one step, because the entry was never touched -- so running out of memory partway through a large object, or having a scratch file swept, answered a failed write by destroying the good value that was already stored. Worse than the failure it was reporting. Ports say which they are: abandonStorageWrite discards the pending write and reports that the entry was left alone, and the default still answers that the caller has to delete. The cached copy goes either way, since the object never reached the storage. The sweep no longer judges a scratch file by its age. Age was the only thing separate processes could agree on, but lastModified is a wall clock reading and a clock that jumps forward makes a file being written this moment look arbitrarily old -- which is how a second process came to delete writes that were still in progress. Each process now holds a lock on a file named for it for as long as it runs, and the sweep asks the filesystem whether the owner of a scratch file is still there. The system drops that lock however a process ends, so it cannot outlive what it stands for, and anything the sweep cannot determine counts as running. What remains of the interval is a rate limit on the monotonic clock, never a judgement about a file. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/CodenameOneImplementation.java | 22 ++ CodenameOne/src/com/codename1/io/Storage.java | 22 +- .../impl/android/AndroidImplementation.java | 213 +++++++++++++----- 3 files changed, 197 insertions(+), 60 deletions(-) diff --git a/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java b/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java index f640731c6e2..2b65176bdb3 100644 --- a/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java +++ b/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java @@ -6800,6 +6800,28 @@ public void setStorageData(Object storageData) { /// - `name`: the name of the storage file public abstract void deleteStorageFile(String 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 + /// + /// #### 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) { + return false; + } + /// Deletes all the files in the application storage public void clearStorage() { String[] l = listStorageEntries(); diff --git a/CodenameOne/src/com/codename1/io/Storage.java b/CodenameOne/src/com/codename1/io/Storage.java index f738b1e377d..7cdcb29eaf4 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -517,11 +517,23 @@ private void failedWrite(String name, Throwable err, boolean includeLogging) { // 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 entry goes, and the cached copy with it. Leaving that 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. - deleteStorageFile(name); + // 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)) { + cache.delete(name); + } else { + deleteStorageFile(name); + } if (includeLogging) { Log.e(err); if (Log.isCrashBound()) { diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 3be1a446258..0936bc1b0fe 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7521,16 +7521,25 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti private static final String STORAGE_SCRATCH_DIR = "cn1-storage-scratch"; /** - * How old a scratch file must be before it is taken for abandoned. + * Suffix of the file each process locks for as long as it is running, so that the + * others can tell whether the writes it left behind are still being written. * - *

Age rather than bookkeeping because an application may run more than one - * process, each with its own copy of this class and so its own idea of what is - * open. A process that swept on behalf of all of them would delete writes another - * process had in flight, and since the failed publish makes writeObject take its - * error path, that would destroy the entry being written. Nothing legitimate - * holds a storage stream open for a day.

+ *

This replaces judging a scratch file by its age. An application may run more + * than one process, each with its own copy of this class and so its own idea of + * what is open, and age was the only thing they all agreed on -- but + * {@code lastModified} is a wall clock reading, and a clock that jumps forward + * makes a file being written this moment look arbitrarily old. A lock says + * whether the writer is there, and the system drops it when a process ends + * however it ends, so it cannot outlive the process it stands for.

*/ - private static final long STORAGE_SCRATCH_MAX_AGE = 24L * 60L * 60L * 1000L; + private static final String STORAGE_LIVE_SUFFIX = ".live"; + + /** + * How long to leave between sweeps. A rate limit rather than a judgement about + * any file, measured on the monotonic clock so that setting the wall clock cannot + * disturb it. + */ + private static final long STORAGE_SWEEP_INTERVAL = 5L * 60L * 1000L; /** * Distinguishes the scratch files of concurrent writes. Paired with the process @@ -7558,6 +7567,14 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti private static RandomAccessFile storageLockHandle; private static FileLock storageLockAcrossProcesses; + /** + * The lock this process holds for as long as it runs, saying that the scratch + * files bearing its process id are still being written. Never released: the + * system takes it back when the process ends. + */ + private static RandomAccessFile storageLiveHandle; + private static FileLock storageLiveLock; + /** * How many nested claims this process has on the cross process lock. A * {@code FileLock} is held by the whole VM and cannot be taken twice, and @@ -7647,10 +7664,9 @@ private static void releaseStorageLock() { new ArrayList(); /** - * When the scratch area is next worth looking at, as a wall clock time. Set to - * the expiry of the youngest file that was kept, so that a process which lives - * for weeks still collects a file that was merely too young the first time it - * looked. Guarded by {@link #storagePublishLock}. + * When the scratch area is next worth looking at, on the monotonic clock. Keeps + * the sweep from running on every write without ever being the thing that decides + * whether a file is abandoned. Guarded by {@link #storagePublishLock}. */ private static long nextStorageScratchSweep; @@ -7728,6 +7744,20 @@ public void clearStorage() { } } + /** + * @inheritDoc + */ + public boolean abandonStorageWrite(String name) { + synchronized (storagePublishLock) { + for (int iter = 0; iter < openStorageWrites.size(); iter++) { + openStorageWrites.get(iter).cancel(name); + } + } + // the entry is untouched until a write is published, so a write that never + // gets that far leaves whatever was stored exactly as it was + return true; + } + /** * @inheritDoc */ @@ -7776,37 +7806,131 @@ public int getStorageEntrySize(String name) { */ private void sweepStorageScratchFiles() { synchronized (storagePublishLock) { - long now = System.currentTimeMillis(); + long now = android.os.SystemClock.elapsedRealtime(); if (now < nextStorageScratchSweep) { return; } - // looked at again when the youngest file that survived this pass comes of - // age. Setting a flag once instead would let a file that was a few minutes - // old at the first write of a long lived process sit there for the life of - // that process, however large it is. - long nextSweep = now + STORAGE_SCRATCH_MAX_AGE; + nextStorageScratchSweep = now + STORAGE_SWEEP_INTERVAL; try { - File[] abandoned = storageScratchDir().listFiles(); - if (abandoned != null) { - for (int iter = 0; iter < abandoned.length; iter++) { - if (isStorageLockFile(abandoned[iter]) - || isOpenStorageWrite(abandoned[iter])) { - continue; - } - long expires = abandoned[iter].lastModified() + STORAGE_SCRATCH_MAX_AGE; - if (expires > now) { - nextSweep = Math.min(nextSweep, expires); - } else if (!abandoned[iter].delete()) { - com.codename1.io.Log.p("Could not remove the abandoned storage " - + "scratch file " + abandoned[iter]); - } + File dir = storageScratchDir(); + File[] files = dir.listFiles(); + if (files == null) { + return; + } + int mine = android.os.Process.myPid(); + for (int iter = 0; iter < files.length; iter++) { + if (isStorageLockFile(files[iter])) { + continue; + } + int owner = storageScratchOwner(files[iter].getName()); + // this process knows what it is doing without asking, and never + // tries to lock its own liveness file, which it already holds + if (owner < 0 || owner == mine || isProcessWriting(dir, owner)) { + continue; + } + if (!files[iter].delete()) { + com.codename1.io.Log.p("Could not remove the abandoned storage " + + "scratch file " + files[iter]); } } } catch (Throwable t) { // a sweep that fails costs disk space, never correctness com.codename1.io.Log.e(t); } - nextStorageScratchSweep = nextSweep; + } + } + + /** + * The process a file in the scratch directory belongs to. + * + * @param fileName the name of the file + * @return the process id, or -1 if the name does not carry one + */ + private static int storageScratchOwner(String fileName) { + String pid; + if (fileName.endsWith(STORAGE_LIVE_SUFFIX)) { + pid = fileName.substring(0, fileName.length() - STORAGE_LIVE_SUFFIX.length()); + } else { + int digest = fileName.indexOf('-'); + int counter = digest < 0 ? -1 : fileName.indexOf('-', digest + 1); + if (counter < 0) { + return -1; + } + pid = fileName.substring(digest + 1, counter); + } + try { + return Integer.parseInt(pid); + } catch (NumberFormatException err) { + return -1; + } + } + + /** + * Whether the given process is still running, and so may still be writing the + * scratch files that carry its id. + * + *

Asked of the filesystem rather than of {@code /proc}, which since Android 9 + * shows a process only itself. A lock that can be taken is one nobody is holding. + * Anything unexpected counts as running, since deleting another process's work on + * a guess is the one outcome worth avoiding here.

+ * + * @param dir the scratch directory + * @param pid the process to ask about + * @return true if that process appears to be running + */ + private static boolean isProcessWriting(File dir, int pid) { + File live = new File(dir, pid + STORAGE_LIVE_SUFFIX); + if (!live.exists()) { + return false; + } + RandomAccessFile handle = null; + FileLock held = null; + try { + handle = new RandomAccessFile(live, "rw"); + held = handle.getChannel().tryLock(); + return held == null; + } catch (Throwable t) { + return true; + } finally { + try { + if (held != null) { + held.release(); + } + if (handle != null) { + handle.close(); + } + } catch (Throwable t) { + com.codename1.io.Log.e(t); + } + } + } + + /** + * Says, for as long as this process runs, that the scratch files carrying its + * process id are still being written. + * + * @param dir the scratch directory + */ + private static void claimStorageLiveness(File dir) { + synchronized (storagePublishLock) { + if (storageLiveLock != null) { + return; + } + try { + storageLiveHandle = new RandomAccessFile( + new File(dir, android.os.Process.myPid() + STORAGE_LIVE_SUFFIX), "rw"); + storageLiveLock = storageLiveHandle.getChannel().lock(); + } catch (Throwable t) { + com.codename1.io.Log.e(t); + try { + if (storageLiveHandle != null) { + storageLiveHandle.close(); + } + } catch (Throwable ignored) { + com.codename1.io.Log.e(ignored); + } + storageLiveHandle = null; + } } } @@ -7849,28 +7973,6 @@ private static boolean isStorageLockFile(File file) { return STORAGE_LOCK_FILE.equals(file.getName()); } - /** - * Whether the given scratch file belongs to a write this process has open. - * - *

Age alone would not tell: {@code lastModified} is a wall clock reading, and - * a clock that jumps forward -- an automatic correction, say -- can make a file - * being written this moment look like a day old. What this process is doing it - * knows exactly, so it never has to guess.

- * - *

The caller must hold {@link #storagePublishLock}.

- * - * @param file a file in the scratch directory - * @return true if a write in this process is using it - */ - private static boolean isOpenStorageWrite(File file) { - for (int iter = 0; iter < openStorageWrites.size(); iter++) { - if (openStorageWrites.get(iter).scratch.equals(file)) { - return true; - } - } - return false; - } - /** * The start of the name of every scratch file for the given entry. * @@ -7974,6 +8076,7 @@ private static final class StorageOutputStream extends OutputStream { throw new IOException("Could not create the storage scratch directory " + dir); } + claimStorageLiveness(dir); // the digest of the entry lets another process find and cancel this write. // The process id separates concurrent processes, whose counters both start // from the beginning, and the counter separates writes within one. From 69b149099e18b58b8b37fb53a5673f6937d3834c Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:12:00 +0300 Subject: [PATCH 11/17] Leave the liveness markers alone when clearing the storage Clearing threw away every file in the scratch directory, the liveness markers among them. A process whose marker is taken from underneath it goes on holding the lock, so it never notices and never makes the name again, and from then on every other process reads it as gone and feels free to delete the writes it has in flight. Same shape as the lock file two changes ago, so the exclusion is now a question about markers rather than about one name: clearing throws away the writes and nothing else, and the sweep stays the only place a marker is removed, once its owner is known to be gone. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/android/AndroidImplementation.java | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 0936bc1b0fe..42d42b8e829 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7945,7 +7945,7 @@ private static void discardAllScratchFiles() { return; } for (int iter = 0; iter < scratch.length; iter++) { - if (!isStorageLockFile(scratch[iter]) && !scratch[iter].delete()) { + if (!isStorageMarkerFile(scratch[iter]) && !scratch[iter].delete()) { com.codename1.io.Log.p("Could not cancel the storage write " + scratch[iter]); } @@ -7973,6 +7973,24 @@ private static boolean isStorageLockFile(File file) { return STORAGE_LOCK_FILE.equals(file.getName()); } + /** + * Whether the given file is one of the markers the processes keep about + * themselves, rather than a write in progress. + * + *

Clearing the storage throws away the writes, and nothing else. A process + * whose liveness file was taken from underneath it goes on holding the lock, so + * it never notices and never makes the name again, and from then on every other + * process reads it as gone and feels free to delete the writes it has in flight. + * The sweep is the one place a liveness file is removed, and only once its owner + * is known to be gone.

+ * + * @param file a file in the scratch directory + * @return true if the file is a marker rather than a pending write + */ + private static boolean isStorageMarkerFile(File file) { + return isStorageLockFile(file) || file.getName().endsWith(STORAGE_LIVE_SUFFIX); + } + /** * The start of the name of every scratch file for the given entry. * From e2680be8d2d3cd80f55f18ee21a3e3739601169e Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:24:17 +0300 Subject: [PATCH 12/17] Give up the write that failed, not every write to the same entry A failed write was abandoned by entry name, so when two threads wrote the same entry and one of them failed, the other was given up along with it: it skipped publication, closed without complaint, and its writeObject reported success for a value that had been discarded. The write is named by its stream now -- Storage keeps the one the implementation handed it -- so only the write that failed is given up. The sweep passes over anything carrying its own process id, on the grounds that a process knows its own work. Android hands a process id out again once its holder is gone, so after a crash or a reboot that assumption covered files an earlier incarnation had abandoned, and they would have sat there for good. Claiming liveness now clears whatever is already present under this process's id, which happens before its first write, when it owns nothing and anything there must belong to the incarnation before it. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/CodenameOneImplementation.java | 7 ++- CodenameOne/src/com/codename1/io/Storage.java | 24 +++++++--- .../impl/android/AndroidImplementation.java | 46 +++++++++++++++++-- 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java b/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java index 2b65176bdb3..ba4c8aa1993 100644 --- a/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java +++ b/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java @@ -6814,11 +6814,16 @@ public void setStorageData(Object storageData) { /// /// - `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) { + public boolean abandonStorageWrite(String name, OutputStream writing) { return false; } diff --git a/CodenameOne/src/com/codename1/io/Storage.java b/CodenameOne/src/com/codename1/io/Storage.java index 7cdcb29eaf4..b1d10a13982 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -471,18 +471,24 @@ 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 implementation's own 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 = createOutputStream(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 writing = d; //NOPMD CloseResource + DataOutputStream closing = d; //NOPMD CloseResource d = null; - writing.close(); + 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, @@ -490,10 +496,10 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { // 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, err, includeLogging); + failedWrite(name, writing, err, includeLogging); throw err; } catch (Exception err) { - failedWrite(name, err, includeLogging); + failedWrite(name, writing, err, includeLogging); return false; } finally { Util.getImplementation().cleanup(d); @@ -507,11 +513,15 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { /// /// - `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, Throwable err, boolean includeLogging) { + 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 @@ -529,7 +539,7 @@ private void failedWrite(String name, Throwable err, boolean includeLogging) { // 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)) { + if (Util.getImplementation().abandonStorageWrite(name, writing)) { cache.delete(name); } else { deleteStorageFile(name); diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 42d42b8e829..8f98df3544e 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7747,14 +7747,18 @@ public void clearStorage() { /** * @inheritDoc */ - public boolean abandonStorageWrite(String name) { - synchronized (storagePublishLock) { - for (int iter = 0; iter < openStorageWrites.size(); iter++) { - openStorageWrites.get(iter).cancel(name); + public boolean abandonStorageWrite(String name, OutputStream writing) { + // this write and no other. Every write to the entry used to be given up + // together, so a second thread writing the same entry had its value quietly + // discarded and was told the write had succeeded. + if (writing instanceof StorageOutputStream) { + synchronized (storagePublishLock) { + ((StorageOutputStream) writing).cancel(); } } // the entry is untouched until a write is published, so a write that never - // gets that far leaves whatever was stored exactly as it was + // gets that far -- including one whose stream never opened -- leaves whatever + // was stored exactly as it was return true; } @@ -7920,6 +7924,7 @@ private static void claimStorageLiveness(File dir) { storageLiveHandle = new RandomAccessFile( new File(dir, android.os.Process.myPid() + STORAGE_LIVE_SUFFIX), "rw"); storageLiveLock = storageLiveHandle.getChannel().lock(); + discardEarlierIncarnation(dir); } catch (Throwable t) { com.codename1.io.Log.e(t); try { @@ -7973,6 +7978,37 @@ private static boolean isStorageLockFile(File file) { return STORAGE_LOCK_FILE.equals(file.getName()); } + /** + * Removes whatever a previous process left behind under this process's id. + * + *

Android hands out a process id again once the process holding it is gone, so + * after a crash or a reboot the files an earlier incarnation abandoned can be + * sitting under the id this one has just been given. The sweep passes over + * anything bearing its own id, on the grounds that a process knows its own work, + * which would leave those files where they are for good. Running this before the + * first write, when this process owns nothing yet, makes that assumption true: + * anything already here under its id belongs to the incarnation before it.

+ * + *

The caller must hold {@link #storagePublishLock}.

+ * + * @param dir the scratch directory + */ + private static void discardEarlierIncarnation(File dir) { + File[] files = dir.listFiles(); + if (files == null) { + return; + } + int mine = android.os.Process.myPid(); + for (int iter = 0; iter < files.length; iter++) { + if (!isStorageMarkerFile(files[iter]) + && storageScratchOwner(files[iter].getName()) == mine + && !files[iter].delete()) { + com.codename1.io.Log.p("Could not remove the abandoned storage scratch " + + "file " + files[iter]); + } + } + } + /** * Whether the given file is one of the markers the processes keep about * themselves, rather than a write in progress. From db1ac3ec8914287230b8b4a6f1fd8de3e76245e7 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:38:48 +0300 Subject: [PATCH 13/17] Keep the streaming API streaming; replace the entry only for whole values The atomic replacement was applied to createStorageOutputStream, which backs a public streaming API where a caller may hold the stream open and read back what it has flushed. The log writer does exactly that: it keeps the stream for CN1Log__$ open for the life of the application and only flushes, while sendLog reads that entry behind its back. An entry that appears only on close left the log unreadable, sendLog uploading the previous session or nothing at all, and every line written since the process started lost when it ended. That is a regression this change made, and a bad one, since the log is what a crash is diagnosed from. The two are separated now. createStorageOutputStream writes into the entry as it always did, and gains only the flush on close that Android does not do, which changes nothing about when what is written can be read. writeObject asks for the other form, where the whole value is assembled elsewhere and put in place as one step. That matches what each is for: a value written in one go is never wanted half written, and a stream held open is no use if nothing can read it. The lock helpers report through Android's log rather than ours. Ours writes through storage, so a failure to take the lock would have been reported by a path that comes back through the same code with the depth still at zero, fails again the same way, and does not stop until the stack does. Adds the test that the round of fixes for it never had: a write that fails leaves the value that was already stored where it was. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/CodenameOneImplementation.java | 25 ++++++ CodenameOne/src/com/codename1/io/Storage.java | 9 +- .../impl/android/AndroidImplementation.java | 90 +++++++++++++++++-- .../java/com/codename1/io/StorageTest.java | 30 +++++-- .../TestCodenameOneImplementation.java | 21 +++++ 5 files changed, 159 insertions(+), 16 deletions(-) diff --git a/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java b/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java index ba4c8aa1993..1db52ef4163 100644 --- a/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java +++ b/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java @@ -6800,6 +6800,31 @@ 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. /// diff --git a/CodenameOne/src/com/codename1/io/Storage.java b/CodenameOne/src/com/codename1/io/Storage.java index b1d10a13982..8f3f96681b3 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -474,11 +474,16 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { OutputStream writing = null; //NOPMD CloseResource DataOutputStream d = null; //NOPMD CloseResource try { - // the implementation's own stream is kept, because that is what names the + // asks for a stream that becomes the entry only once it is closed, which + // createOutputStream deliberately is not: that one is the public streaming + // API, where a caller may hold the stream open and read back what it has + // flushed, and the log writer does exactly that. + // + // The implementation's own 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 = createOutputStream(name); + writing = Util.getImplementation().createStorageOutputStream(name, true); d = new DataOutputStream(writing); Util.writeObject(o, d); // closed here rather than left to the finally, because closing is where diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 8f98df3544e..8aee815bff2 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7614,7 +7614,10 @@ private static void lockStorageAcrossProcesses() { storageLockAcrossProcesses = storageLockHandle.getChannel().lock(); } } catch (Throwable t) { - com.codename1.io.Log.e(t); + // android's log, not ours: the default log writer is a storage stream, + // so reporting this through it would come back through here with the + // depth still at zero and fail the same way, again and again + Log.e("CodenameOne", "Could not lock the storage", t); releaseStorageLock(); } } @@ -7643,7 +7646,7 @@ private static void releaseStorageLock() { storageLockAcrossProcesses.release(); } } catch (Throwable t) { - com.codename1.io.Log.e(t); + Log.e("CodenameOne", "Could not release the storage lock", t); } storageLockAcrossProcesses = null; try { @@ -7651,7 +7654,7 @@ private static void releaseStorageLock() { storageLockHandle.close(); } } catch (Throwable t) { - com.codename1.io.Log.e(t); + Log.e("CodenameOne", "Could not close the storage lock", t); } storageLockHandle = null; } @@ -7755,21 +7758,89 @@ public boolean abandonStorageWrite(String name, OutputStream writing) { synchronized (storagePublishLock) { ((StorageOutputStream) writing).cancel(); } + // such a write leaves the entry untouched until it is published, so + // whatever was stored is still there + return true; } - // the entry is untouched until a write is published, so a write that never - // gets that far -- including one whose stream never opened -- leaves whatever - // was stored exactly as it was - return true; + // a stream that never opened cannot have touched anything either. Anything + // else wrote into the entry itself and the caller has to clear up after it. + return writing == null; } /** * @inheritDoc + * + *

Writes into the entry, as it always has. A caller may hold this open and + * expect what it flushes to be readable meanwhile -- the log writer keeps one for + * the life of the application and sendLog reads the entry behind its back -- so + * an entry that appeared only on close would leave the log unreadable and lose + * everything written since the process started. What can be given here without + * changing when the entry appears is the flush that Android does not do on + * close.

*/ public OutputStream createStorageOutputStream(String name) throws IOException { + return new SyncingStorageOutputStream(getContext().openFileOutput(name, 0)); + } + + /** + * @inheritDoc + */ + public OutputStream createStorageOutputStream(String name, boolean replaceWhenClosed) + throws IOException { + if (!replaceWhenClosed) { + return createStorageOutputStream(name); + } sweepStorageScratchFiles(); return new StorageOutputStream(name); } + /** + * Forces a stream onto the device as it closes, which Android does not do by + * itself, without changing anything about when what is written becomes visible. + */ + private static final class SyncingStorageOutputStream extends OutputStream { + private final FileOutputStream out; + private boolean closed; + + SyncingStorageOutputStream(FileOutputStream out) { + this.out = out; + } + + @Override + public void write(int b) throws IOException { + out.write(b); + } + + @Override + public void write(byte[] b) throws IOException { + out.write(b); + } + + @Override + public void write(byte[] b, int off, int len) throws IOException { + out.write(b, off, len); + } + + @Override + public void flush() throws IOException { + out.flush(); + } + + @Override + public void close() throws IOException { + if (closed) { + return; + } + closed = true; + try { + out.flush(); + out.getFD().sync(); + } finally { + out.close(); + } + } + } + /** * @inheritDoc */ @@ -7926,13 +7997,14 @@ private static void claimStorageLiveness(File dir) { storageLiveLock = storageLiveHandle.getChannel().lock(); discardEarlierIncarnation(dir); } catch (Throwable t) { - com.codename1.io.Log.e(t); + // android's log for the same reason as above + Log.e("CodenameOne", "Could not claim the storage liveness file", t); try { if (storageLiveHandle != null) { storageLiveHandle.close(); } } catch (Throwable ignored) { - com.codename1.io.Log.e(ignored); + Log.e("CodenameOne", "Could not close the liveness file", ignored); } storageLiveHandle = null; } diff --git a/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java b/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java index 5bb6953995d..605f0f08218 100644 --- a/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java @@ -137,13 +137,33 @@ void failedWriteLeavesNothingBehindInTheCache() { String key = "unwritable"; assertTrue(storage.writeObject(key, "the value that is really stored")); - // Object is not one of the supported types, so Util.writeObject throws and - // the entry is removed. The cached copy has to go with it, otherwise reads - // keep answering from memory until the app is restarted. + // Object is not one of the supported types, so Util.writeObject throws. The + // object is cached before the write is attempted, so it has to be dropped + // again: otherwise reads answer from memory with a value the storage never + // took, and the write only looks to have failed once the app is restarted. + Object neverStored = new Object(); + assertFalse(storage.writeObject(key, neverStored, false)); + + Object read = storage.readObject(key); + assertNotSame(neverStored, read); + assertEquals("the value that is really stored", read); + } + + @EdtTest + void aFailedWriteLeavesThePreviousValueInPlace() { + String key = "keeps"; + assertTrue(storage.writeObject(key, "the value that was already stored")); + storage.clearCache(); + + // Object is not a supported type, so serialization fails partway. An + // implementation that only replaces the entry when the write is closed never + // touched it, so answering the failure by deleting it would throw away a good + // value on account of a write that never reached the storage. assertFalse(storage.writeObject(key, new Object(), false)); - assertFalse(storage.exists(key)); - assertNull(storage.readObject(key)); + storage.clearCache(); + assertTrue(storage.exists(key)); + assertEquals("the value that was already stored", storage.readObject(key)); } @EdtTest diff --git a/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java b/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java index 0849a6d3084..35986cd8d3a 100644 --- a/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java +++ b/maven/core-unittests/src/test/java/com/codename1/testing/TestCodenameOneImplementation.java @@ -3110,6 +3110,23 @@ public void deleteStorageFile(String name) { } } + /** + * Models an implementation that only replaces the entry when the write is closed, + * so a write given up before then leaves whatever was stored untouched. + * + * @param name the entry being written + * @param writing the stream handed out for the write that failed + * @return true, since this double never writes into the entry itself + */ + @Override + public boolean abandonStorageWrite(String name, OutputStream writing) { + if (writing instanceof StorageOutput) { + ((StorageOutput) writing).discard(); + return true; + } + return writing == null; + } + /** * Makes storage writes fail at the point an entry would be published, which is * where an implementation that replaces the entry in one step does the writing. @@ -4389,6 +4406,10 @@ void discard(String entry) { } } + void discard() { + discarded = true; + } + @Override public void close() throws IOException { super.close(); From 63a28ba310c15c0dc283ebd51d3ec4dd0aa318ee Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:50:27 +0300 Subject: [PATCH 14/17] Sweep under the lock the other processes start their writes with Finding an owner gone and deleting its files are two steps, and Android hands a process id out again the moment its holder is gone. Between the liveness check and the delete, a process could be given the id just examined, say it was running and begin a write, and have the sweep unlink the write it had only just started -- or the very file it had said it was alive with, after which every later sweep would take it for gone and delete whatever it was writing. Creating a scratch file was already serialized between processes; the sweep and the liveness claim were not. Both take that lock now, so deciding an id is dead cannot land in the middle of another process claiming it. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/android/AndroidImplementation.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 8aee815bff2..525ea612c43 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -7886,6 +7886,14 @@ private void sweepStorageScratchFiles() { return; } nextStorageScratchSweep = now + STORAGE_SWEEP_INTERVAL; + // under the lock the other processes take to start a write or to say they + // are running. Finding an owner gone and then deleting its files are two + // steps, and a process id is handed out again the moment its holder is + // gone: without this a process could be given the id just examined, say so + // and start writing, and have this sweep delete the write it had only just + // begun -- or the very file it had said it was alive with, after which + // every later sweep would take it for gone. + lockStorageAcrossProcesses(); try { File dir = storageScratchDir(); File[] files = dir.listFiles(); @@ -7911,6 +7919,8 @@ private void sweepStorageScratchFiles() { } catch (Throwable t) { // a sweep that fails costs disk space, never correctness com.codename1.io.Log.e(t); + } finally { + unlockStorageAcrossProcesses(); } } } @@ -7991,6 +8001,10 @@ private static void claimStorageLiveness(File dir) { if (storageLiveLock != null) { return; } + // under the same lock the sweep takes, so that saying this process is + // running and clearing what the last holder of its id left behind cannot + // land in the middle of another process deciding that id is gone + lockStorageAcrossProcesses(); try { storageLiveHandle = new RandomAccessFile( new File(dir, android.os.Process.myPid() + STORAGE_LIVE_SUFFIX), "rw"); @@ -8007,6 +8021,8 @@ private static void claimStorageLiveness(File dir) { Log.e("CodenameOne", "Could not close the liveness file", ignored); } storageLiveHandle = null; + } finally { + unlockStorageAcrossProcesses(); } } } From 87a517e6f8e502db54b4c4cddd7e9a566c71b402 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:55:54 +0300 Subject: [PATCH 15/17] Clear the liveness lock when the claim fails, and let the tidy up fail alone Closing the handle gives up the lock, but the field saying this process held it was left set. Every later claim then returned as though the lock were still held, while no lock existed -- so every other process read the .live file as unlocked, took this process for gone, and was free to delete the writes it had in flight. Both fields are cleared now. The failure that prompted it could only happen because clearing up after the previous holder of this process id ran inside the same try as the claim itself. It has its own now: the claim has already succeeded by that point and is not worth giving up because a leftover file would not delete. Those keep until a later sweep. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/android/AndroidImplementation.java | 41 +++++++++++++------ 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index 525ea612c43..f604be868ea 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -8006,21 +8006,36 @@ private static void claimStorageLiveness(File dir) { // land in the middle of another process deciding that id is gone lockStorageAcrossProcesses(); try { - storageLiveHandle = new RandomAccessFile( - new File(dir, android.os.Process.myPid() + STORAGE_LIVE_SUFFIX), "rw"); - storageLiveLock = storageLiveHandle.getChannel().lock(); - discardEarlierIncarnation(dir); - } catch (Throwable t) { - // android's log for the same reason as above - Log.e("CodenameOne", "Could not claim the storage liveness file", t); try { - if (storageLiveHandle != null) { - storageLiveHandle.close(); - } - } catch (Throwable ignored) { - Log.e("CodenameOne", "Could not close the liveness file", ignored); + storageLiveHandle = new RandomAccessFile( + new File(dir, android.os.Process.myPid() + STORAGE_LIVE_SUFFIX), "rw"); + storageLiveLock = storageLiveHandle.getChannel().lock(); + } catch (Throwable t) { + // android's log for the same reason as above + Log.e("CodenameOne", "Could not claim the storage liveness file", t); + try { + if (storageLiveHandle != null) { + storageLiveHandle.close(); + } + } catch (Throwable ignored) { + Log.e("CodenameOne", "Could not close the liveness file", ignored); + } + // the lock as well as the handle: closing the handle gives up the + // lock, and a lock this process still believed it held is one it + // would never take again, which leaves every other process reading + // it as gone and free to delete the writes it has in flight + storageLiveHandle = null; + storageLiveLock = null; + return; + } + try { + discardEarlierIncarnation(dir); + } catch (Throwable t) { + // separately, because the claim above has already succeeded and + // clearing up after whoever held this id last is not worth giving + // it up for. The leftovers keep until a later sweep. + Log.e("CodenameOne", "Could not clear the earlier incarnation", t); } - storageLiveHandle = null; } finally { unlockStorageAcrossProcesses(); } From 42f5ee3119d93a357f5491abe39a0b42d80739bd Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Sat, 22 Aug 2026 00:09:08 +0300 Subject: [PATCH 16/17] Keep writeObject going through a custom Storage's own streams setStorageInstance is there so an application can wrap the bytes, seamless encryption being the case its own documentation names, and writeObject has always gone through the subclass's createOutputStream. Asking the platform for the stream directly walked past that: the value went to the store unwrapped while reads went on expecting otherwise, so what came back could not be decoded. writeObject asks createOutputStreamForWrite now, which is overridable. Its default hands back the platform's replace-on-close stream only for Storage itself; a subclass keeps the stream it has always been given, and can override the new method to wrap that one and have both. The test writes through a Storage that inverts every byte and reads it back, so a write that skipped the wrapper fails to decode. Verified against the previous revision, where it fails. Co-Authored-By: Claude Opus 5 (1M context) --- CodenameOne/src/com/codename1/io/Storage.java | 46 ++++++++--- .../java/com/codename1/io/StorageTest.java | 80 +++++++++++++++++++ 2 files changed, 116 insertions(+), 10 deletions(-) diff --git a/CodenameOne/src/com/codename1/io/Storage.java b/CodenameOne/src/com/codename1/io/Storage.java index 8f3f96681b3..4a41dc5e996 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -474,16 +474,11 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { OutputStream writing = null; //NOPMD CloseResource DataOutputStream d = null; //NOPMD CloseResource try { - // asks for a stream that becomes the entry only once it is closed, which - // createOutputStream deliberately is not: that one is the public streaming - // API, where a caller may hold the stream open and read back what it has - // flushed, and the log writer does exactly that. - // - // The implementation's own 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 = Util.getImplementation().createStorageOutputStream(name, true); + // 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 @@ -511,6 +506,37 @@ public boolean writeObject(String name, Object o, boolean includeLogging) { } } + /// 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. /// diff --git a/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java b/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java index 605f0f08218..762e891b945 100644 --- a/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java +++ b/maven/core-unittests/src/test/java/com/codename1/io/StorageTest.java @@ -28,7 +28,11 @@ import org.junit.jupiter.api.BeforeEach; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import java.util.Vector; import static org.junit.jupiter.api.Assertions.*; @@ -149,6 +153,82 @@ void failedWriteLeavesNothingBehindInTheCache() { assertEquals("the value that is really stored", read); } + @EdtTest + void writeObjectGoesThroughACustomStoragesOwnStreams() { + // setStorageInstance exists so an application can wrap the bytes, seamless + // encryption being the case the API documents. writeObject has always gone + // through the subclass's createOutputStream, and has to keep doing so: + // writing past the wrapper leaves bytes the matching reader cannot decode. + final List wrapped = new ArrayList(); + Storage custom = new Storage() { + @Override + public OutputStream createOutputStream(String name) throws IOException { + wrapped.add(name); + return new InvertingOutputStream(super.createOutputStream(name)); + } + + @Override + public InputStream createInputStream(String name) throws IOException { + return new InvertingInputStream(super.createInputStream(name)); + } + }; + Storage.setStorageInstance(custom); + try { + assertTrue(custom.writeObject("wrappedEntry", "the value")); + assertTrue(wrapped.contains("wrappedEntry"), "the write bypassed the subclass"); + + custom.clearCache(); + // only decodes if the write went through the wrapper too + assertEquals("the value", custom.readObject("wrappedEntry")); + } finally { + Storage.setStorageInstance(null); + } + } + + /// Stands in for a Storage that transforms the bytes on their way out. + private static final class InvertingOutputStream extends OutputStream { + private final OutputStream out; + + InvertingOutputStream(OutputStream out) { + this.out = out; + } + + @Override + public void write(int b) throws IOException { + out.write((~b) & 0xff); + } + + @Override + public void flush() throws IOException { + out.flush(); + } + + @Override + public void close() throws IOException { + out.close(); + } + } + + /// The matching reader, which only makes sense of what the writer above produced. + private static final class InvertingInputStream extends InputStream { + private final InputStream in; + + InvertingInputStream(InputStream in) { + this.in = in; + } + + @Override + public int read() throws IOException { + int b = in.read(); + return b < 0 ? b : (~b) & 0xff; + } + + @Override + public void close() throws IOException { + in.close(); + } + } + @EdtTest void aFailedWriteLeavesThePreviousValueInPlace() { String key = "keeps"; From 44905e390a126f31a18e729ce2a2e75b48292959 Mon Sep 17 00:00:00 2001 From: Shai Almog <67850168+shai-almog@users.noreply.github.com> Date: Sat, 22 Aug 2026 00:16:48 +0300 Subject: [PATCH 17/17] Do not let the incarnation cleanup delete a write this process has open Clearing what the last holder of this process id left behind rested on the process owning nothing yet, which is true on the first write and not afterwards: a claim that fails is retried by the next write, and by then there can be writes open under the same id. The cleanup deleted their scratch files, so a write that had serialized perfectly well failed when it came to publish. It leaves the writes it knows about alone now, which it can do exactly rather than by inference. A write still goes ahead when the liveness claim fails, and the reason is written where the decision is. A claim can only fail where the filesystem will not lock, and refusing to write there would turn that into an application unable to store anything at all -- worse than the cost, which is that a process sweeping at that moment may take the write for abandoned and unlink it. That fails the write, honestly, and leaves what was already stored where it is, and the next write claims again. It is the same trade the cross process lock already makes. Co-Authored-By: Claude Opus 5 (1M context) --- .../impl/android/AndroidImplementation.java | 35 +++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java index f604be868ea..f4bb036122b 100644 --- a/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java +++ b/Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java @@ -8088,9 +8088,13 @@ private static boolean isStorageLockFile(File file) { * after a crash or a reboot the files an earlier incarnation abandoned can be * sitting under the id this one has just been given. The sweep passes over * anything bearing its own id, on the grounds that a process knows its own work, - * which would leave those files where they are for good. Running this before the - * first write, when this process owns nothing yet, makes that assumption true: - * anything already here under its id belongs to the incarnation before it.

+ * which would leave those files where they are for good.

+ * + *

Usually this runs before the first write, when the process owns nothing and + * everything under its id must belong to the incarnation before it. That is not + * guaranteed: a claim that fails is retried by the next write, by which time this + * process may have writes of its own open. Those are known exactly and are left + * alone -- deleting one would fail a write that had already been serialized.

* *

The caller must hold {@link #storagePublishLock}.

* @@ -8105,6 +8109,7 @@ private static void discardEarlierIncarnation(File dir) { for (int iter = 0; iter < files.length; iter++) { if (!isStorageMarkerFile(files[iter]) && storageScratchOwner(files[iter].getName()) == mine + && !isOpenStorageWrite(files[iter]) && !files[iter].delete()) { com.codename1.io.Log.p("Could not remove the abandoned storage scratch " + "file " + files[iter]); @@ -8112,6 +8117,23 @@ && storageScratchOwner(files[iter].getName()) == mine } } + /** + * Whether the given scratch file belongs to a write this process has open. + * + *

The caller must hold {@link #storagePublishLock}.

+ * + * @param file a file in the scratch directory + * @return true if a write in this process is using it + */ + private static boolean isOpenStorageWrite(File file) { + for (int iter = 0; iter < openStorageWrites.size(); iter++) { + if (openStorageWrites.get(iter).scratch.equals(file)) { + return true; + } + } + return false; + } + /** * Whether the given file is one of the markers the processes keep about * themselves, rather than a write in progress. @@ -8233,6 +8255,13 @@ private static final class StorageOutputStream extends OutputStream { throw new IOException("Could not create the storage scratch directory " + dir); } + // the write goes ahead whether or not that succeeded. A claim can only + // fail where the filesystem will not lock, and refusing to write would + // turn that into an application that cannot store anything -- far worse + // than what it costs, which is that another process sweeping at that + // moment may take this write for abandoned and unlink it. That fails the + // write, honestly, and leaves what was already stored where it is; the + // next write claims again. Same trade the cross process lock makes. claimStorageLiveness(dir); // the digest of the entry lets another process find and cancel this write. // The process id separates concurrent processes, whose counters both start