saljam / webwormhole

Peer authenticated WebRTC.
BSD 3-Clause "New" or "Revised" License
1.8k stars 91 forks source link

Promisify establish wormhole #86

Closed mfbx9da4 closed 3 years ago

mfbx9da4 commented 3 years ago

@saljam this seems to be more or less working. I've tested bad code and establishing a connection. File sending almost works. The file is received, displayed and downloaded but just hangs on the download part. Probably an issue with triggering the download, maybe it has to do with lack of https on localhost? I checked master and it has the same behaviour as this branch. Perhaps I am missing something or master is broken? Can dig in another time to find the root cause.

mfbx9da4 commented 3 years ago

@saljam I've realized that the reason you need 3 deferred promises is so that the caller code for configuring the peerConnection is evaluated before the offer is created. Using a promise as a synchronization primitive seems a little off here because the caller configuration code is synchronous. I've created three ways of removing the need for the third intermediary promise.

https://codesandbox.io/s/wait-for-caller-promise-zt7y9?file=/src/App.js:1331-1384

Each has its trade-offs. I feel classic callback is the nicest because it is the easiest to understand and the least code. Very close second wait for caller flush event loop. The main benefit of removing the third promise is that the wormhole code is less coupled to the caller code. It doesn't require finish() to be called to march on creating the offer.

The client code would then become something like

const setuppeercon = (pc) => { pc.createDataChannel('....') ... etc }
const w = new Wormhole(url, code, setuppeercon)
const signal = await w.signal()
const fingerprint = await w.finish()

That said, your code works, so if it ain't broke, why fix it.

saljam commented 3 years ago

The file is received, displayed and downloaded but just hangs on the download part.

weird! what browser are you using? current master is running on https://tip.webwormhole.io, and that works for me on macos firefox and chrome, as does localhost.

mfbx9da4 commented 3 years ago

I realized why this wasn't working. It's because I have the following setting in chrome.

image

My hypothesis is that this setting will install the service worker on every reload. However, there is no code to claim control of the client (window) after activation.

self.addEventListener('activate', event => {
  event.waitUntil(clients.claim());
});

I also think I faced this issue earlier in production as I had an older service worker installed. I'll verify tomorrow at some point.

mfbx9da4 commented 3 years ago

The reason is that it wasn't working locally for me is that

window.location = `/_/${receiving.id}`;

triggers chrome dev tools to reload the service worker when Update on reload is enabled. The service worker hangs forever waiting for waitForMetadata(id) because the in memory streams Map is garbage collected. I don't think it's possible for this to happen in production.