[bisq-network/bisq] FileManager can lose writes (#3687)

[Julian Knutsen]

I potential bug was identified in May around losing writes in the FileManager
```
// TODO: this looks like it could cause corrupt data as the savePending is unset before the actual
// save. By moving to after the save there might be some persist operations that are not performed
// and data would be lost. Probably all persist operations should happen sequencially rather than
// skip one when there is already one scheduled
```

After a code review comment on #3640, I wrote a simple test to show the bug. The comments at the end show the interleaving that causes the issue.

```
    @Test
    public void lostWrites() throws IOException, InterruptedException {

        PersistenceProtoResolver persistenceProtoResolver = mock(PersistenceProtoResolver.class);

        when(persistenceProtoResolver.fromProto(any(protobuf.PersistableEnvelope.class))).thenAnswer(invocation -> {
            protobuf.PersistableEnvelope proto = invocation.getArgument(0);
            return SequenceNumberMap.fromProto(proto.getSequenceNumberMap());
        });

        File tempFile = tempFolder.newFile();
        FileManager<SequenceNumberMap> manager = new FileManager<>(
                tempFolder.getRoot(), tempFile, 100, persistenceProtoResolver);


        SequenceNumberMap sequenceNumberMap = new SequenceNumberMap();

        for (int i = 0; i < 100; i++) {
            sequenceNumberMap.put(
                    new P2PDataStorage.ByteArray(new byte[] { (byte) i }), new P2PDataStorage.MapValue(i, i));
            System.out.println("Writing: " + i);
            manager.saveLater(sequenceNumberMap, 1000);

            do {
                // Thread.sleep(1);
            } while (manager.savePending.get());

            SequenceNumberMap fromRead = manager.read(tempFile);
            Assert.assertEquals(sequenceNumberMap.getMap(), fromRead.getMap());

            // saveForLater()
            // -> setAtomicBoolean()
            // -> scheduleThread()
            // saveForLater()
            // -> threadExecutes
            // -> sets Persistable
            // checkAtomicBoolean() -> return
            // write lost
        }
    }
```

Working on a fix now to guarantee that `saveLater` will eventually write to disk if it returns. I don't think we want to write sequentially as the comment suggests. There is code all over the place that uses the fact that batch-writes occur to improve performance.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/issues/3687
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191125/74145515/attachment.html>