digidem / mapeo-core

Library for creating custom geo data and syncronizing via a peer to peer network
23 stars 2 forks source link

v8 peer fixes #87

Closed hackergrrl closed 4 years ago

hackergrrl commented 4 years ago

fixes #84, #83

hackergrrl commented 4 years ago

ooo, good catch! closing sync first would be much safer.

On 05/06 18:06, Karissa McKelvey wrote:

@okdistribute commented on this pull request.

@@ -245,7 +245,11 @@ class Mapeo extends events.EventEmitter { }

close (cb) {

  • this.sync.close(cb)
  • this.osm.core.pause(() => {
  • this.osm.core._logs.close(() => {
  • this.sync.close(cb)

in desktop/mobile we do sync.close first, is there a difference?

-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/digidem/mapeo-core/pull/87#pullrequestreview-407086026

hackergrrl commented 4 years ago

This is finally ready to go!

@gmaclennan @okdistribute This PR changes a lot more code than I had intended to, so I wanted to hold off on releasing this lest it has bugs and semver causes it to impact your MD/MM development. @gmaclennan Can you let me know when you've had a chance to test this with multiple devices on MM?

gmaclennan commented 4 years ago

Hi @noffle I did some more thinking about this and what is needed in the front-end. I think we actually need to re-design the sync UI and Aldo and I have talked about this. The big change we have discussed is showing all known peers in a project, whether they are connected or not. To do this I think we need to implement project support, but it would allow us to show connection state to the user. Without that, I think what we can do is never going to be perfect, but here are some thoughts about states.

NB: I am not sure what a "session" should be. The easiest to implement is from app open to app closed (since that completely closes and restarts the server and all associated state) but on mobile the app can remain open for many days (switching apps only pauses the app, we keep the server state in memory, we just stop close the http server). Currently I do some hacks in the frontend code to make a session appear to be from when the user navigates to the sync screen until they navigate away from the screen.

state 1: Peer connects for the first time in a session. Show to user: show device with a button that enables them to sync.

state 2: Graceful disconnect - e.g. the peer disconnects because it goes offline, the wifi is turned off, or it stops announcing. This disconnect is "graceful" because it does not cause a sync to fail or stop prematurely. show to user: Device should disappear e.g. no longer show in the sync view.

state 3: Starting to sync. Note this is one state in terms of the UI, but it is two states behind the scense: (A) the user has pressed the button and the async request for sync being sent to mapeo-core (we did not track this before on mobile and it makes the buttons seem unresponsive because they do nothing for 50-200ms before the front end gets any response from the backend) and (B) the sync is starting but not progress events have yet been received. show to user: Device with icon that shows that sync is pending (loading animation). The user should not be able to press any button at this time (we do not support "cancel sync" at this time).

state 4: Sync in progress - data is being received / sent. show to user: Device with icon showing progress and/or number of documents/images sent/received (we only show % progress on mobile, but on desktop we also show documents and images progress). The user should not be able to press any button at this time (we do not support "cancel sync" at this time).

state 5: Sync completed successfully show to user: Device with icon / text showing that sync was complete. This should last for the duration of the session because sync can take a long time (10-15 minutes) and the user may look away / do other things, and when they return to the app they should be able to see whether the sync was successful. The UI should show a button that enables the user to sync again (because when syncing other devices multiple syncs might be necessary to get everything in sync).

state 6: Sync error, retry still possible. This could be an error with sync itself, or a peer disconnecting half-way through sync, but then reconnecting. show to user: Show the device with a label/icon indicating that an error happened, with a button that enables them to retry. It is important that the user knows that the sync was unsuccessful, and that they can see the action they can take (retry sync).

state 7: Sync error, retry not possible. I think the only cause of this would be a disconnect during sync. show to user: Unlike the graceful disconnect (2) this disconnect has caused a sync to fail, so the user needs to know this. We should show the peer even though it is disconnected, with an error message/icon. Unfortunately there is no way to retry, so we should not show a retry button. We should add some kind of info button that explains other steps the user can take (e.g. check the wifi is still turned on, check the app is still open on the other device) but we can add this at a later stage, I don't think it's essential immediately.

If a peer does reconnect, then state 7 should change to state 6. At the end of a session states should reset to (1).

We also need to make sure that a peer un-announcing results in it disconnecting and being removed from state, but only if sync is not in progress. We un-announce when the user navigates away from the sync screen. The goal is to give the user control over when they can sync: we can teach that other can only sync with you when you have your sync screen open, so there is not the worry that someone could sync without the users knowledge.

E.g. if device A and device B are on the sync screen, then they should show each other. If the user of device B navigates away from the sync screen, if they are in any state other than (3) or (4), then device B should disappear from the sync screen of device A. If device B navigates away from the sync screen during states (3) or (4) then device B should continue to appear on the sync screen of device A. When sync completes without error, if the user of device B is not on the sync screen still, then device B should disappear from the sync screen of device A.

Some of this logic might be too hard to implement, in that we might be better getting the most important fixes done now and add some of the more complex logic later, particularly the logic around leaving a swarm by navigating away and how that affects state.

hackergrrl commented 4 years ago

Added @okdistribute's close fixes, and this also now exposes a peer.connected property.

This PR is turning into a v8 feature/fix grab-bag, which is a signal to me that this ought to get merged so we can resume regular development & PRs on top of the mainline v8 branch.

hackergrrl commented 4 years ago

Minor release published: 8.2.0.

hackergrrl commented 4 years ago

@gmaclennan Good notes! I think it'd be good if they got stored in an issue somewhere until we're ready to revisit this, so it doesn't get buried & forgotten.