callbacks -> async / await

[dirkmc]

BREAKING CHANGE: All places in the API that used callbacks are now replaced with async/await

[jacobheun]

We'll need to refactor the tests and api at https://github.com/libp2p/interface-transport to async/await for this change.

[dirkmc]

Yes, definitely. Alan is working on some documentation in the websockets PR: libp2p/js-libp2p-websockets#82

[alanshaw]

This is super cool @dirkmc 🚀 - I'm going to fully review this later today but one thing I noticed skimming through is that the AbortSignal passed to dial can only be used to abort the socket during connection. This differs from what I've done in libp2p/js-libp2p-websockets#82 - where the signal is ALSO passed to both sides of the socket after the connection is opened so you can abort later. We should discuss.

cc @jacobheun

[dirkmc]

@alanshaw what would be the use case for aborting after the connection has been made (as opposed to calling close() on the connection)?

[alanshaw]

Good question.

If you abort the stream it’ll throw an error that you can catch and inspect. If you simply close the stream, you’ll exit the loop and you’ll not know if the stream finished or if it was aborted.

[dirkmc]

I'm not sure I understand - it looks from the code like it will catch the AbortError and close the stream

[jacobheun]

what would be the use case for aborting after the connection has been made (as opposed to calling close() on the connection)

The main case for this is parallel dials in the Switch. If I dial to a peer that I have 3 TCP addresses for, ideally I would dial to all 3 in parallel to connect as quickly as possible. The first connection to succeed, would result in me aborting the other 2 dials. It's entirely possible that by the time I have the first connection, 1 or both of the other connections could also be made. Instead of detecting that difference, it would be nice to just call abort on the other dials and be done.

Note: As we currently don't have the option to cleanly abort dials, we only dial 1 address at a time.

[dirkmc]

Makes sense 👍

I will make the change in the code here and add a test to interface-transport

[alanshaw]

I'm not sure I understand - it looks from the code like it will catch the AbortError and close the stream

On the sink side I think there's not much else you can do, it's the sink so it's the "end of the pipeline"...but the source is abortable with the same signal and I was referring to this in the above comment (I wrote it on my phone so it was perhaps too brief).

pipe(
  dataToSendToServer,
  duplexSocket,
  source => { // our sink (receiver for server responses)
    for await (const chunk of source) {
      // do something with the response from the server...
    }
    // we now get here because duplexSocket.close() was called somewhere 
    // else (connection mgr?) and we exit the loop.
    // this is a werid state because we might not have received all we wanted
    // from the server....but no error was thrown, what do we do now...?
  }
)

So, using the abort signal allows us to catch that error and know that the connection was aborted because of some reason on our side of the connection.

This feels to me like a useful distinction to make. This has to be proven and we're on totally new ground here so questioning the reasoning is really good. If we ever want to do reconnect because of sleep/network change/network disruption (which we do 😜) it'll be useful to not try to reconnect when the connection manager aborts the connection purposefully or when the node is being stopped (for examples).

@jacobheun has the concrete use case of course ;)

[dirkmc]

@alanshaw I see what you're saying, that does seem like a useful distinction, thanks!

[dirkmc]

I added the ability to abort after connect. Depends on libp2p/interface-transport#49 (new tests in interface-transport)

I also made a change such that if the sink is destroyed while we are writing to it, we just end the stream. Does that make sense to you guys or do you think it should throw an error?

[alanshaw]

There's a lot of code in src/listener.js managing multiple listeners. If every transport module now need to deal with tacking multiple listeners this needs to be refactored into something generic that can be reused. At the moment I'm finding it difficult to distinguish between code that's creating and closing them and code that's managing/tracking them.

At the moment I'm in favour of keeping the listen API more simple like it was before (see libp2p/interface-transport#46 (comment) for reasoning) and dealing with the multiple listeners outside of the transport modules themselves to reduce repetition and complexity.

[dirkmc]

@jacobheun what do you think about @alanshaw's suggestion that we revert back to listening on a single address? Should we revert the PR to add multiple listeners?

[jacobheun]

@dirkmc per my comment libp2p/interface-transport#46 (comment), I agree with reverting at the interface and here. It sounds like everyone is in agreement so we can move forward with those reversions.

[dirkmc]

Ok thanks @jacobheun I will apply the reverts

[dirkmc]

Depends on libp2p/interface-transport#51

[dirkmc]

@jacobheun @alanshaw I made a change such that if the sink is destroyed while we are writing to it, we just log a warning and end the stream. Does that make sense to you guys or do you think it should throw an error?

[jacobheun]

It should throw, otherwise we might unknowingly continue trying to write to a destroyed connection.

[alanshaw]

It should throw, otherwise we might unknowingly continue trying to write to a destroyed connection.

I think it can't throw, and I have been meaning to get back to this PR for a long time to explain properly why. Please do not merge until I can explain! 🙏

[jacobheun]

FYI, I have rebased this against the latest master.

[jacobheun]

Closing in favor of #112