Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

refactor: switch to async iterators #82

Closed
wants to merge 17 commits into from
Closed

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Mar 29, 2019

BREAKING CHANGE: Switch to using async/await and async iterators. The transport and connection interfaces have changed.

Dials are now abortable! Pass an AbortSignal as signal to the options to abort the connection whenever you like!

For example, abort during dial:

const controller = new AbortController

// Already aborted!
controller.abort()

try {
  const socket = await transport.dial(ma, { signal: controller.signal })
} catch (err) {
  if (err.type === 'aborted') {
    // dial was aborted while connecting...
  }
}

For example, abort after dial:

const { collect } = require('streaming-iterables')
const pipe = require('it-pipe')

const controller = new AbortController

// Successful dial
const socket = await transport.dial(ma, { signal: controller.signal })

const infiniteRandom = {
  [Symbol.asyncIterator] () { return this },
  next () { return new Promise(r => setTimeout(() => r(Math.random()), 1000)) }
}

// Abort after 5s
setTimeout(() => controller.abort(), 5000)

try {
  await pipe(infiniteRandom, socket, collect)
} catch (err) {
  if (err.type === 'aborted') {
    // connection was aborted after connected
  }
}

Note: there is an adapter class that allows use of the new API via the old one, usage: const Websockets = require('libp2p-websockets/src/adapter')

Interface Transport

dial(ma<Multiaddr>, [options<Object>]): Promise<Connection>

Dial to a peer at the given multiaddr, returns a promise that is resolved when the connection has opened. Throws if connection error or any other error opening the connection, including if the connection is aborted.

The promise resolves to a Connection (which is a duplex object) that can be used to communicate with the peer.

Options
  • signal - an AbortSignal that can be used to abort the dial. You can obtain one from an AbortController

createListener([options<Object>], [handler<Function<Connection>>]): Listener

Create a new listener, where incoming connections are handled by handler. The handler function is called for each new incoming connection. It is passed a Connection (which is a duplex object). Note the handler param is optional and can be registered later by listening for the 'connected' event.

filter(mas<Multiaddr[]>): Multiaddr[]

Filter the multiaddrs this transport can dial.

Events

listening

After the listener has started listening.

connected

For every new connection. The event handler is passed a Connection (which is a duplex object).

close

After the listener has stopped listening.

Interface Listener

listen(): Promise

getAddrs(): Promise<Multiaddr[]>

Interface Connection

sink<Function>

https://gist.github.com/alanshaw/591dc7dd54e4f99338a347ef568d6ee9#sink-it

source<Iterable>

https://gist.github.com/alanshaw/591dc7dd54e4f99338a347ef568d6ee9#source-it

getObservedAddrs(): Promise<Multiaddr[]>

close(): Promise


  • test/adapter/compliance/* are just a copy of the old interface-transport tests

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@ghost ghost assigned alanshaw Mar 29, 2019
@ghost ghost added the status/in-progress In progress label Mar 29, 2019
Alan Shaw added 3 commits April 1, 2019 14:27
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@jacobheun
Copy link
Contributor

With the change over to async/await something that may be valuable to look at adding to dial is AbortController support. Currently in js-libp2p there is no way to abort a dial, which can lead to connection leaks if not properly accounted for. For example, if I dial to a peer that has 4 websocket addresses, we usually want to dial all 4 in parallel as some of them may be inaccurate. Once I get a valid connection back I want to be able to abort the other 3.

It also makes it much easier to properly close outbound dials when shutting a node down.

@dirkmc
Copy link

dirkmc commented Apr 2, 2019

Good idea @jacobheun 👍

Alan Shaw added 3 commits April 3, 2019 10:38
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw alanshaw marked this pull request as ready for review April 18, 2019 09:16
@alanshaw
Copy link
Member Author

After some thinking, abort controller support was actually super easy to add using abortable-iterator. The only tricky bit was allowing abort before connection is established.

Should be good for review.

Async await changes merged but not released :(

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
src/index.js Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/adapter.js Show resolved Hide resolved
src/listener.js Outdated Show resolved Hide resolved
test/adapter/compliance/dial-test.js Outdated Show resolved Hide resolved
@dirkmc
Copy link

dirkmc commented Apr 18, 2019

interface-transport now has a few extra tests for cancelling dials, which require a new connector object to be passed to setup(). See the libp2p-tcp async/await PR for an example. I also copied the Adapter class into interface-transport, example here.

src/adapter.js Outdated
@@ -0,0 +1,81 @@
'use strict'

const { Connection } = require('interface-connection')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do the async/await update to interface-connection for this? It should be a pretty quick change and will prevent us from needing to come back and update this later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dirkmc do you want to send a PR for this?

cc @vasco-santos (I know you were thinking of working on it this week)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what fields / methods should be required on the interface, I opened an issue to discuss: libp2p/interface-connection#28

Alan Shaw added 5 commits April 18, 2019 13:31
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
Alan Shaw added 2 commits April 18, 2019 15:30
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@olizilla
Copy link

olizilla commented Aug 8, 2019

This PR would also get us of pull-ws which depends on ws@1.1.5 which is triggering github vulnrability warnings on projects that depend on js-ipfs...

websockets/ws@c4fe466

"aegir": "^18.2.1",
"chai": "^4.2.0",
"dirty-chai": "^2.0.1",
"interface-transport": "~0.3.7",
"interface-transport": "github:libp2p/interface-transport#master",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If #master, why not released? //cc @jacobheun

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's wrong, this is outdated. We're tackling tcp first and then will circle back to this and the other transports.

@jacobheun
Copy link
Contributor

Superseded by #92

@jacobheun jacobheun closed this Sep 30, 2019
@jacobheun jacobheun deleted the refactor/async-iterators branch September 30, 2019 10:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants