diff --git a/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java b/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java index f640731c6e2..1db52ef4163 100644 --- a/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java +++ b/CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java @@ -6800,6 +6800,58 @@ public void setStorageData(Object storageData) { /// - `name`: the name of the storage file public abstract void deleteStorageFile(String name); + /// Creates an output stream that holds the whole new value and only becomes the + /// entry once it is closed, for an implementation that can offer that. + /// + /// This is not what `createStorageOutputStream` does, and the two are kept apart + /// on purpose. That one backs a public streaming API: a caller may hold it open + /// for the life of the application and expect what it has flushed to be readable + /// meanwhile, which is exactly how the log writer uses it. An entry that appears + /// only on close would leave such a caller writing to something nothing can read. + /// + /// Writing a whole value at once has no such expectation, and gains what the + /// streaming form cannot be given: the entry is never seen partly written, and + /// what was there before survives a write that fails. + /// + /// #### Parameters + /// + /// - `name`: the storage file name + /// + /// #### Returns + /// + /// an output stream, which replaces the entry as one step when closed if this + /// implementation is able to + public OutputStream createStorageOutputStream(String name, boolean replaceWhenClosed) throws IOException { + return createStorageOutputStream(name); + } + + /// Gives up a write that failed partway through, for an implementation that can + /// throw one away without the entry it was replacing being any the worse for it. + /// + /// An implementation that writes into the entry itself leaves half an object + /// behind when a write fails, and the only way to be rid of that is to delete the + /// entry, so the default here reports that it cannot help. One that prepares the + /// new value elsewhere and puts it in place in a single step has not touched the + /// entry at all, and deleting it would throw away a good value on account of a + /// write that never reached it. + /// + /// #### Parameters + /// + /// - `name`: the name of the storage file being written + /// + /// - `writing`: the stream this implementation returned for the write that + /// failed, or null if it never opened. The write is named by its stream rather + /// than by its entry so that a second write to the same entry, which may be + /// perfectly healthy, is not given up along with it + /// + /// #### Returns + /// + /// true if the pending write was discarded and the entry left as it was, false if + /// the caller still has to delete the entry to be rid of a partial write + public boolean abandonStorageWrite(String name, OutputStream writing) { + return false; + } + /// Deletes all the files in the application storage public void clearStorage() { String[] l = listStorageEntries(); 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..4a41dc5e996 100644 --- a/CodenameOne/src/com/codename1/io/Storage.java +++ b/CodenameOne/src/com/codename1/io/Storage.java @@ -471,25 +471,118 @@ public boolean writeObject(String name, Object o) { public boolean writeObject(String name, Object o, boolean includeLogging) { name = fixFileName(name); cache.put(name, o); + OutputStream writing = null; //NOPMD CloseResource DataOutputStream d = null; //NOPMD CloseResource try { - d = new DataOutputStream(createOutputStream(name)); + // the stream is kept, because that is what names the write to be given up + // if this one fails. Giving up by entry name would reach every write to + // that entry, and a second thread writing the same one would be told its + // value was stored after it had been discarded. + writing = createOutputStreamForWrite(name); + d = new DataOutputStream(writing); Util.writeObject(o, d); + // closed here rather than left to the finally, because closing is where + // an implementation that writes the entry in one step does the writing. + // From the finally the failure would reach cleanup(), which logs and + // swallows it, and this method would report a write that never landed. + // handed off so the finally does not close it a second time + DataOutputStream closing = d; //NOPMD CloseResource + d = null; + closing.close(); return true; + // Errors are caught too, and rethrown. An OutOfMemoryError partway + // through a large object would otherwise leave the entry to the finally, + // which closes the stream, and an implementation that publishes an entry + // as it closes would put those few bytes in place of a good entry. The + // error still reaches the caller, it just does not take the entry with it. + } catch (Error err) { + failedWrite(name, writing, err, includeLogging); + throw err; } catch (Exception err) { - if (includeLogging) { - Log.e(err); - if (Log.isCrashBound()) { - Log.sendLog(); - } - } - Util.getImplementation().deleteStorageFile(name); + failedWrite(name, writing, err, includeLogging); return false; } finally { Util.getImplementation().cleanup(d); } } + /// Creates the stream that `writeObject` writes a whole value into. + /// + /// This is deliberately not `createOutputStream`. That one is the public + /// streaming API, where a caller may hold the stream open and read back what it + /// has flushed -- the log writer keeps one for the life of the application -- so + /// it goes on writing into the entry. A whole value has no such expectation, and + /// so can be given what streaming cannot: it is assembled away from the entry and + /// put in place as one step, where the platform is able to, so the entry is never + /// seen half written and a write that fails leaves what was stored alone. + /// + /// A `Storage` installed through `setStorageInstance` to wrap the bytes -- the + /// seamless encryption that extension point exists for -- has always had + /// `writeObject` go through its own `createOutputStream`, and still does: + /// bypassing it would write those bytes past the encryption while reads went on + /// expecting it. Such a subclass can override this method to take the stronger + /// guarantee as well, wrapping what the superclass returns. + /// + /// #### Parameters + /// + /// - `name`: the storage file name, already normalized + /// + /// #### Returns + /// + /// the stream to write the value into + protected OutputStream createOutputStreamForWrite(String name) throws IOException { + if (getClass() != Storage.class) { + return createOutputStream(name); + } + return Util.getImplementation().createStorageOutputStream(name, true); + } + + /// Gives up on a write that failed partway through, leaving neither a partial + /// entry on the storage nor the object that never reached it in the cache. + /// + /// #### Parameters + /// + /// - `name`: the storage file name, already normalized + /// + /// - `writing`: the implementation's stream for this write, or null if it never + /// opened + /// + /// - `err`: what went wrong + /// + /// - `includeLogging`: whether the failure may be logged, which is unsafe during + /// app initialization + private void failedWrite(String name, OutputStream writing, Throwable err, + boolean includeLogging) { + // before the logging, which can fail in its own right. Reporting an + // OutOfMemoryError means building a message and a stack trace, so a second + // failure there would carry off the rest of this method, and the write left + // open would be published by the finally that closes it. + // + // The cached copy goes either way. Leaving it behind hid the failure for the + // rest of the session: every read was answered from memory with the object + // that never reached the storage, and the entry only turned up missing after + // the app was restarted. + // + // Whether the entry itself has to go depends on where the failed write went. + // An implementation that writes into the entry has left half an object there + // and deleting is the only way to be rid of it. One that assembles the value + // elsewhere and puts it in place in a single step never touched the entry, so + // deleting it would answer a write that failed by throwing away the value + // that was already stored -- which is worse than the failure itself, and is + // what running out of memory partway through a large object used to do. + if (Util.getImplementation().abandonStorageWrite(name, writing)) { + cache.delete(name); + } else { + deleteStorageFile(name); + } + if (includeLogging) { + Log.e(err); + if (Log.isCrashBound()) { + Log.sendLog(); + } + } + } + /// Reads the object from the storage, returns null if the object isn't there /// /// The sample below demonstrates the usage and registration of the `com.codename1.io.Externalizable` interface: 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..f4bb036122b 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; @@ -7506,18 +7507,338 @@ public String[] getHeaderFields(String name, Object connection) throws IOExcepti } + /** + * Directory holding storage writes still in progress. + * + *

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"; + + /** + * 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. + * + *

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 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 + * id, since a second process counts from the beginning as well. + */ + 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(); + + /** + * 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; + + /** + * 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 + * 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()) { + // 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 = storageLockHandle.getChannel().lock(); + } + } catch (Throwable 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(); + } + } + 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) { + Log.e("CodenameOne", "Could not release the storage lock", t); + } + storageLockAcrossProcesses = null; + try { + if (storageLockHandle != null) { + storageLockHandle.close(); + } + } catch (Throwable t) { + Log.e("CodenameOne", "Could not close the storage lock", t); + } + storageLockHandle = null; + } + + /** + * 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(); + + /** + * 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; + /** * @inheritDoc */ public void deleteStorageFile(String name) { - getContext().deleteFile(name); + synchronized (storagePublishLock) { + 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(); + } + } + } + + /** + * 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 */ + 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. + lockStorageAcrossProcesses(); + try { + for (int iter = 0; iter < openStorageWrites.size(); iter++) { + openStorageWrites.get(iter).cancel(); + } + discardAllScratchFiles(); + super.clearStorage(); + } finally { + unlockStorageAcrossProcesses(); + } + } + } + + /** + * @inheritDoc + */ + 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(); + } + // such a write leaves the entry untouched until it is published, so + // whatever was stored is still there + 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 getContext().openFileOutput(name, 0); + 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(); + } + } } /** @@ -7554,6 +7875,556 @@ public int getStorageEntrySize(String name) { return (int)new File(getContext().getFilesDir(), name).length(); } + /** + * 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) { + long now = android.os.SystemClock.elapsedRealtime(); + if (now < nextStorageScratchSweep) { + 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(); + 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); + } finally { + unlockStorageAcrossProcesses(); + } + } + } + + /** + * 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; + } + // 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 { + try { + 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); + } + } finally { + unlockStorageAcrossProcesses(); + } + } + } + + /** + * 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 (!isStorageMarkerFile(scratch[iter]) && !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); + } + } + + /** + * 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()); + } + + /** + * 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.

+ * + *

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}.

+ * + * @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 + && !isOpenStorageWrite(files[iter]) + && !files[iter].delete()) { + com.codename1.io.Log.p("Could not remove the abandoned storage scratch " + + "file " + files[iter]); + } + } + } + + /** + * 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. + * + *

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. + * + *

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. + * + * @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() 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); + } + + /** + * 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 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; + private boolean cancelled; + + 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); + } + // 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 + // 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 + // exists but which a concurrent deleteStorageFile cannot see to cancel, + // and that write would rename itself over the entry that was deleted. + synchronized (storagePublishLock) { + lockStorageAcrossProcesses(); + try { + this.out = new FileOutputStream(scratch); + openStorageWrites.add(this); + } finally { + unlockStorageAcrossProcesses(); + } + } + } + + /** + * 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 + * {@link #storagePublishLock}. + * + * @param entry the entry being deleted + */ + void cancel(String entry) { + if (name.equals(entry)) { + cancelled = true; + } + } + + @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 { + try { + out.flush(); + out.getFD().sync(); + } finally { + out.close(); + } + publish(); + } finally { + synchronized (storagePublishLock) { + openStorageWrites.remove(this); + } + if (scratch.exists() && !scratch.delete()) { + com.codename1.io.Log.p("Could not remove the storage scratch file " + + scratch); + } + } + } + + /** + * 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) { + 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; + } + if (scratch.renameTo(target)) { + syncStorageDirectory(target.getParentFile()); + 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(); + } + } + } + } + + /** + * 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/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()); 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..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 @@ -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; @@ -5,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.*; @@ -108,4 +135,132 @@ 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. 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 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"; + 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)); + + storage.clearCache(); + assertTrue(storage.exists(key)); + assertEquals("the value that was already stored", 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 119bf0319a9..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; @@ -12,9 +35,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 +175,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..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 @@ -98,6 +98,8 @@ */ 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<>(); @@ -3101,6 +3103,38 @@ 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); + } + } + + /** + * 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. + * + * @param failsOnClose whether closing a storage output stream should fail + */ + public void setStorageWriteFailsOnClose(boolean failsOnClose) { + storageWriteFailsOnClose = failsOnClose; } public void putStorageEntry(String name, byte[] data) { @@ -4359,15 +4393,34 @@ public String toString() { private final class StorageOutput extends ByteArrayOutputStream { private final String name; + private boolean discarded; StorageOutput(String name) { this.name = name; + openStorageWrites.add(this); + } + + void discard(String entry) { + if (name.equals(entry)) { + discarded = true; + } + } + + void discard() { + discarded = true; } @Override public void close() throws IOException { super.close(); - storageEntries.put(name, toByteArray()); + 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()); + } } }