fix: sync bugs with new v8 connected and started properties

[okdistribute]

Found these bugs while updating Mapeo Desktop to the latest v8.

This will be published as @mapeo/core v8.2.2

Sync peers should always emit the close event

Added a failing (now passing!) test where peer.started was never getting set, which lead to the replication-complete state never firing.

For example, this state should not exist, where replication-progress is the state and started is false.

The test also captures this by making sure peer.started is set on the first progress event.

The culprit?

self._activeSyncs gets into an incorrect state when a peer would disconnect without syncing, and then reconnect.

Peer A joins
Peer B joins
Peer A closes
-1 activeSyncs
Peer A joins
Peer B syncs with Peer A
-1 activeSyncs + 1 = 0
0 activeSyncs means that sync-start is never emitted

Sending replicateNetwork too quickly would sometimes not do anything

If peer.connected the sync start doesn't go through and nothing happens.

Now, it sends an error if the peer is not connected when replicate is called.

See https://github.com/digidem/mapeo-core/pull/94/files#diff-39367c259f0252fc4bc764d86564411bR234-R239

An alternative way to do this would be to create an internal queue of syncronizations that have been requested, and once a peer reconnects, see if a replication has already been requested and drain that queue. But this seemed to be unnecessary as consumer applications should not allow syncronization if the peer is disconnected. (although it would be a nice to have convenience for developer ux)

Syncronizing and then disconnecting would not properly set peer.connected = false

Peer A joins
Peer B joins
Peer B and A syncronize
Peer A quits
peer.connected is true during the down event. It should be false as the peer is now disconnected.

This is fixed by setting peer.connected = false in the onClose function before the down event is emitted with the peer -- otherwise a consumer application will think that the peer is still connected when it is not.

See https://github.com/digidem/mapeo-core/pull/94/files#diff-39367c259f0252fc4bc764d86564411bR422

[hackergrrl]

Great catch @okdistribute 🎉

[okdistribute]

@noffle please review!
I added this because if you hit replicateNetwork twice in a row, it would throw an error immediately. This gives the consumer a bit more of what they expect

[hackergrrl]

Still applicable? I see you removed the sync queue.

[hackergrrl]

Still applicable? I see you removed the sync queue.

[okdistribute]

Yeah I removed it!

[okdistribute]

There were some edge cases and it became clear that it wasn't necessary now although a nice to have, better to push it out the door

[okdistribute]

@gmaclennan is the mobile app also using node 12 to build?

[gmaclennan]

@okdistribute the mobile app is running on Node 10 (I assume you mean the Node version it is running mapeo-core on, vs. the Node version used to build the app?). The latest nodejs-mobile is using Node v12.6.3 and we should be able to update to that without issues, I had only been holding off until we are sure that the stack runs ok on Node v12 (I remember that we had issues with level etc. not supporting Node v10 before). If we know the mapeo-core stack works with Node v12 then I can update MM.

[gmaclennan]

I haven't fully parsed the logic here, but on a quick read through this all looks great. In a future PR, would it be possible to queue a sync request until a handshake completes instead of throwing an error? I feel like that is a slightly confusing reason for an error for the user. Ideal behaviour would be:

1. Click sync before handshake
2. Handshake completes
3. Continue sync if handshake was successful, throw error if handshake failed.

But glad this fixes the other errors

[gmaclennan]

This and _isclosed don't need to be methods (they don't reference this, and aren't public methods) so they could be standalone functions. But that's just a stylistic thing.

[okdistribute]

Well, the peer object is encapsulated by the SyncState class, so these methods should be in the SyncState (where the peer object is created and managed) to maintain that. Ideally, the peer object would be it's own class and these methods would live there.

Currently PeerState is operating more like a struct or complex data type rater than a class, so it doesn't feel appropriate to add methods there either.