refactor!: use more steps for invites

[EvanHahn]

I recommend reviewing this with whitespace changes disabled.

See #447 for details.

I plan to add invite cancelations in an upcoming PR.

Closes #279.
Closes #363.
Closes #446.

[gmaclennan]

I ran out of time today to complete the review - there is a lot of code here to process. Some initial comments here, which might be resolved in other parts of the code that I have not read yet.

[gmaclennan]

Didn't review the tests, but went through all the other code, and it all looks good other than a couple of small changes. Opinions about deferred promises can be ignored for now.

A thought looking at this, a utility function like this would be helpful:

async function onceSatisfied(emitter, eventName, check, { signal }) {
  return new Promise((res, rej) => {
    signal.once('abort', () => {
       emitter.off(eventName, onEvent)
    })
    emitter.on(eventName, function onEvent (...args) {
        const satisfied = checked.apply(null, args)
        if (!satisfied) return
        emitter.off(eventName, onEvent)
        res.apply(null, args)
    })
  })

We've got quite a few places where we want a await once(), but waiting for an event with specific arguments. A helper like that would make writing the code without the deferred promise a bit easier.

[gmaclennan]

Just reviewed the onceSatisfied changes and left some comments - I think these need fixed.
Do you know why CI is failing?

[gmaclennan]

I think this is needed to avoid an uncaught error if #sendAcceptResponse below throws, since this could throw in a tick after that once this function context no longer exists.

Suggested change

	projectDetailsPromise.catch(noop)

[gmaclennan]

(we do want projectDetailsProject to be able to throw, hence this is on a separate line)

[EvanHahn]

I can easily change this, but onceSatisfied should never reject, even if aborted—it will resolve null. Therefore, I don't think this needs to be caught. Happy to change any of that, including my understanding.

[gmaclennan]

ah, right, I hadn't fully parsed the implementation of onceSatisfied. I see it won't reject, but notice it can result in an uncaught error, if the check function throws, e.g. in this case it would throw if somehow projectDetailsPeerId or other params were a number or undefined (timingSafeEqual would throw a TypeError).

I think it's always best to assume that async functions throw, and make sure that they are caught, rather than relying on the assumption that they won't throw, because down the line it's easy to add some code that results in them throwing, and it's hard to retain the memory of "this function must never throw".

In a case like this I'm not a huge fan of returning null to indicate abort or an error from a helper like this, although I think that's just a personal preference. I prefer to throw, and if I want to use null as my check then I can do fnThatMightThrow().catch(() => null) or I can choose to handle the error a different way.

[gmaclennan]

TL;DR, onceSatisfied needs to wrap check() in a try...catch, and either reject or return null if it throws. My preference is to throw, but fine with keeping res(null) if preferred. I think we should have the .catch(noop) anyway, so anyone refactoring / changing code doesn't have to remember which async functions can throw and which can't.

[EvanHahn]

onceSatisfied now throws. I added .catch(noop) to its calls. See 759e5c5.

[gmaclennan]

I think you need to abort the abortSignal in this timeout?

[EvanHahn]

That's done here:

https://github.com/digidem/mapeo-core-next/blob/210ab8bca6ed1f415daf144396b64f8354a8d56d/src/member-api.js#L155-L157

[gmaclennan]

yeah but if we never receive an invite response, await receiveInviteResponsePromise will never resolve, and the cleanup functions will never be called. Or am I missing something?

[EvanHahn]

I think that sequence of events would be:

1. We set up the receive invite abort controller.
   https://github.com/digidem/mapeo-core-next/blob/25d3c073cb9992ea66f0ecf381bcf5efb2819cea/src/member-api.js#L154
2. We immediately add it to our list of cleanup functions.
   https://github.com/digidem/mapeo-core-next/blob/25d3c073cb9992ea66f0ecf381bcf5efb2819cea/src/member-api.js#L155-L157
3. The timeout fires and handleAborted now throws.
   https://github.com/digidem/mapeo-core-next/blob/25d3c073cb9992ea66f0ecf381bcf5efb2819cea/src/member-api.js#L121-L125
4. handleAborted is called, which throws an error.
   https://github.com/digidem/mapeo-core-next/blob/25d3c073cb9992ea66f0ecf381bcf5efb2819cea/src/member-api.js#L179
5. The error is caught and cleaned up.
   https://github.com/digidem/mapeo-core-next/blob/25d3c073cb9992ea66f0ecf381bcf5efb2819cea/src/member-api.js#L211-L213

More broadly: even if this code is technically correct¹, it's hard to follow. I spent a good amount of time trying to make this clearer but I know I missed at least one good idea: onceSatisfied. I'll try to think of further ways to simplify this, as it's hard to understand.

Footnotes

1. which it may not be ↩

[gmaclennan]

The code that is going to
potentially take a long time and we want to timeout is awaiting the invite to be accepted on line 181. If that never resolves, then I don't see how the timeout can abort it, since the abort only happens if the code keeps executing.

[EvanHahn]

You're totally right. Fixed in dbfb6a9.

[EvanHahn]

Do you know why CI is failing?

Yes, silly reason: I canceled it. I did this because I knew I was gonna keep working, so I didn't want to waste GitHub's time.

[EvanHahn]

Addressed the next round of comments!

[EvanHahn]

Got offline approval to merge this, so I'm going for it.