automerge / automerge-repo

MIT License
463 stars 51 forks source link

Leave means leave #146

Open acurrieclark opened 1 year ago

acurrieclark commented 1 year ago

There is an uncommon and unlikely-to-effect-the-real-world bug which occurs with the BroadcastChannel network adapter.

When something like Hot Module Replacement causes a page to be initialised a large number of times within a short window of time, any Repos on the page are instantiated with a new peerId. All of them join the BroadcastChannel network and they all start syncing with each other. It doesn't take long for this loop to get very much out of hand.

I think this could likely be mitigate by implementing leave on the adapter, and then exposing a close or leave method on Repo itself to allow the user to manually close network connections.

I have tested the issue the latest pre-1.0 release and it also exists then, so this hasn't been introduced by any of our recent efforts.

pvh commented 1 year ago

There's an unload handler that might help here -- we can send a "leave" message then. The stuff with weakRefs in the MessageChannel implementation is also relevant -- we could change the BroadcastChannel to just handle introductions and then pass peerwise MessageChannels over it which would disappear when the tab died. That sort of assumes that we care about this deisgn... which... maybe? Maybe we should just show that using messagechannel/sharedworkers is easy enough and do away with this.

I don't have a super strong opinion; what do you think?

pvh commented 1 year ago

i just got this