automerge / automerge-repo-rs

MIT License
39 stars 6 forks source link

Remove unnenecessary sending of first sync messages #45

Closed gterzian closed 1 year ago

gterzian commented 1 year ago

Removing this because I have no idea why it's needed, tests are passing and bakery example runs fine. Must have been a leftover from some previous setup where sending the messages there was necessary?

alexjg commented 1 year ago

I think this might be necessary but the tests pass due to an interaction with storage.

The intention of that branch is something like this: "if the document is bootstrapping then we want to load it from storage and request it from the network, if we don't have this document info in Repo::documents then we haven't done either of these things yet and so we should enqueue a storage request and some sync messages".

Now, if you remove the branch then what happens is that we add the DocumentInfo to Repo::documents, enqueue a sstorage.get and then stop. Then, when the storage future completes (which it does immediately without yielding in the tests because we return futures::ready() from DummyStorage::get and SimpleStorage::get) then a storage event is sent to Repo::wake_sender which ultimately ends up enqueuing sync messages when it gets handled in Repo::run. You can test this by applying the following diff and seeing that the test_requesting_document_connected_peers test fails (with the change suggested):

--- a/test_utils/src/storage_utils.rs
+++ b/test_utils/src/storage_utils.rs
@@ -54,7 +54,8 @@ impl InMemoryStorage {

 impl Storage for InMemoryStorage {
     fn get(&self, id: DocumentId) -> BoxFuture<'static, Result<Option<Vec<u8>>, StorageError>> {
-        futures::future::ready(Ok(self.documents.lock().get(&id).cloned())).boxed()
+        futures::future::pending().boxed()
+        //futures::future::ready(Ok(self.documents.lock().get(&id).cloned())).boxed()
     }

Now, it's arguable that we should not try sending sync messages until we've resolved the storage future so maybe that's fine?

gterzian commented 1 year ago

if the document is bootstrapping then we want to load it from storage and request it from the network, if we don't have this document info in Repo::documents then we haven't done either of these things yet and so we should enqueue a storage request and some sync messages

Yes that is correct, thanks for the reminder.

By the way I think we should consider removing the load API, reasons explained and discussion opened at https://github.com/automerge/spanreed/issues/46