Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnenecessary sending of first sync messages #45

Conversation

gterzian
Copy link
Collaborator

@gterzian gterzian commented Sep 4, 2023

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?

@gterzian gterzian requested a review from alexjg September 4, 2023 11:39
@alexjg
Copy link
Collaborator

alexjg commented Sep 4, 2023

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
Copy link
Collaborator Author

gterzian commented Sep 8, 2023

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 #46

@gterzian gterzian closed this Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants