Make switch a state machine #278

jacobheun · 2018-09-26T13:54:05Z

Getting this pushed up for visibility while I work on cleanup and add some additional tests.

Goal

This PR updates libp2p-switch to be a state machine, including its connections #24.

Updates

Switch will now emit start, stop, and error events. Callback legacy support is included.
Incoming connections will now ensure crypto is done before other muxing. Resolves #239 .
Outgoing and Incoming connections are now state machines. Rather than being internally, automatically driven, they are driven by dialer.js and handler.js respectively. This allows finer control over transitions and helps provide clarity around when and where errors occur.
Adds in some error standardization via the err-codes module
Fixed readme links

TODOs

There is redundant logic with the new Connection FSMs that needs to be cleaned up.
Add more, thorough tests for each of the connection FSMs.
Ensure incoming connections are being properly reused
General TODO cleanup
Add class-is module and remove instanceof logic, as it's unreliable across package versions.

Thoughts on Future Additions

As error codes are added throughout the system, have the FSMs handle fatal and non fatal errors appropriately.
Add hooks for new connections to allow modules like the connection manager, to make intelligent decisions about those connections (dial backoffs, etc).

References

Libp2p State Machine#98

Required by libp2p/js-libp2p#257

jacobheun · 2018-09-26T13:58:08Z

@diasdavid @pgte @vasco-santos I've still got some cleanup to do per my notes in the PR overview, but I'd love to get some feedback on the overall implementation of the FSM. There may be some considerations I've missed that we could roll into this update.

pgte

LGTM, with some questions / comments.

pgte · 2018-09-27T08:56:56Z

src/connection/index.js

+        if (circuitTried) {
+          this.emit('error', new Error(`No available transports to dial peer ${this.theirB58Id}!`))
+          return this._state('disconnect')
+        }


These last two conditions can dictate the end of the dialing attempt. As a client of the switch API, I'd like to now the reason why a dial failed. "No available transport" or "Circuit not enabled" doesn't tell me much of why the other transports failed.

As a remedy, I suggest that we keep track of the failing reasons and make them somehow available to the switch user.

As an alternative, emit a warning-like event like "connection attempt failed", containing an error object as payload.

I like the warning option as that would provide the user with more information about the various attempts, even if connections succeed on a later transport.

With the current structure the switch would need to emit the events. Perhaps the payload should also include the peerInfo so users understand where it came from. Something like:

_switch.emit('error:connection_attempt_failed', { peerInfo: theirPeerInfo, error: ... })

I see. This dialing is triggered by a switch.dial(), which is callback-based..
Having the switch emit these events independently of the dial() call means that, for the API user, it will be hard to correlate a call with these warnings.
Usually I solve this by either returning an even stream or an event emitter. In this case, I think that something like a dialFSM would be something useful to consider...

That's a good point. Currently Libp2p doesn't return anything from its dial methods so returning an event emitter there from a Switch dialFSM would enable us to keep backwards compatibility more easily while we transition the system.

For js-ipfs we'd also need to make sure to return that emitter from swarm.connect().

@pgte I've added a dialsFSM method to the switch, that can be used instead of dial. Instead of a connection, it will return the fsm. I've added some tests for it to show the events and functionality it has https://github.com/libp2p/js-libp2p-switch/pull/278/files#diff-080e21482eea9ffdd7ee511eb71d3522.

The main things to note:
The raw connection is still returned in the callback passed to dialFSM

Users can listen on error events to get fatal messages

Users can listen on error:connection_attempt_failed events to get a list of errors for failed transport dials, these are not fatal to the connection.

Users can listen on close events to know when the specific connection is closed.

Users can call fsm.close() to close that specific connection.

Are there other events or methods that would be helpful to have? We can always wait to add other things if there aren't higher priority nice to haves.

@jacobheun That's great! Other than successful connection attempt (which you can infer from having the "connected" callback called), I don't see any other so far.

src/connection/index.js

pgte · 2018-09-27T09:02:14Z

src/dialer.js

+      connection.on('connected', () => connection.protect())
+      connection.on('private', () => connection.encrypt())
+      connection.on('encrypted', () => connection.upgrade())
+      connection.on('muxed', () => {


Can all these events occur more than once?

No, they should only occur once, I'll update these.

vasco-santos

Seems good to me! Left just a few minor comments

vasco-santos · 2018-09-27T10:13:35Z

src/dialer.js

+    const peerInfo = getPeerInfo(peer, _switch._peerBook)
+    const b58Id = peerInfo.id.toB58String()
+
+    log(`${_switch._peerInfo.id.toB58String().slice(0, 8)} dial request to ${b58Id.slice(0, 8)} with protocol ${protocol}`)


Verify if protocol exists and if not add unknown instead of allowing undefined

vasco-santos · 2018-09-27T10:18:40Z

src/index.js

+      log('The switch has stopped')
+      this.emit('stopped')
+    })
+    this.state.on('error', (err) => this.emit('error', err))


Aiming to ease debuggability, I think we should log the error here as well.

vasco-santos · 2018-09-27T10:19:26Z

src/index.js

@@ -167,7 +167,7 @@ class Switch extends EE {

  /**
   * If a muxed Connection exists for the given peer, it will be closed
-   * and its reference on the Switch will be removed.
+   * and its reference on the Switch and will be removed.


typo? I think it was correct before 🤔

src/index.js

vasco-santos · 2018-09-27T10:36:50Z

src/index.js

+
+        conn.muxer.end((err) => {
+          // If OK things are fine, and someone just shut down
+          if (err && err.message !== 'Fatal error: OK') {


log the error?

vasco-santos · 2018-09-27T10:41:46Z

src/limit-dialer/queue.js

    this._dialWithTimeout(transport, addr, (err, conn) => {
      if (err) {
-        log('work:error')
+        log(`${transport.constructor.name}:work:error`, err)


use log.error here

fix: better support custom handlers

jacobheun · 2018-10-04T19:51:02Z

After reviewing the crypto logic in switch as it currently is, we're not actually supporting multiple crypto modules on dial, only on listen. This needs to be corrected, but after doing some small changes in this branch I think it should wait until this is done as it's a significant change. That will enable us to review those changes more easily to ensure they're done properly.

Also, since secio is currently our only crypto library in libp2p this isn't a critical need.

daviddias · 2018-10-17T21:30:03Z

README.md

-  - [`switch.stop(callback)`](#swarmclosecallback)
-  - [`switch.connection`](#connection)
+  - [`switch.dial(peer, protocol, callback)`](#switchdialpeer-protocol-callback)
+  - [`switch.dialFSM(peer, protocol, callback)`](#switchdialfsmpeer-protocol-callback)


better just mimic js-libp2p and do dial and dialProtocol

@diasdavid are you suggesting to add dialProtocol in addition to the new dialFSM or as a naming replacement? I think having dial handle calls with and without a protocol is reasonable overloading. Replacing it wouldn't be good as it has a different callback footprint to give users access to the ConnectionFSM for finer control/handling.

I've exposed the same dialFSM method in the libp2p branch

We don't need to block this PR for this. What I mean is that given that libp2p (the module) is just an extension to the libp2p-switch (the dialing machine) that adds it other features, it would be cool if they both expose the same dialing/hangup APIs.

That makes sense. Ideally I'd like to deprecate dialProtocol in libp2p and just use dial. Having both feel unnecessary. I'll propose the change in an issue there and if dialProtocol ends up staying we can add that in here.

jacobheun · 2018-10-18T14:05:52Z

@vasco-santos this should be ready for final review. I have this branch integrated in libp2p/js-libp2p#257, which should also be ready for review.

vasco-santos

@jacobheun all in all looks good to me 🙂

Left just minor notes

vasco-santos · 2018-10-19T08:53:46Z

README.md

+  - [`switch.unhandle(protocol)`](#switchunhandleprotocol)
+  - [`switch.start(callback)`](#switchstartcallback)
+  - [`switch.stop(callback)`](#switchstopcallback)
+  - [`switch.connection`](#switchconnection)


Can we please alphabetize the API? I believe this helps when looking for documentation.

vasco-santos · 2018-10-19T10:26:26Z

src/index.js

+   * Issues a start on the Switch state.
+   *
+   * @param {function} callback deprecated: Listening for the `error` and `start` events are recommended
+   * @fires Switch#started


@fires Switch#started ?

vasco-santos · 2018-10-19T10:37:23Z

src/index.js

+   * A listener that will turn off all running services and listeners
+   *
+   * @private
+   * @fires Switch#error


same as above

jacobheun · 2018-10-19T11:06:01Z

@vasco-santos I updated the order in the readme and fixed the improper usage of jsdocs @fires on individual methods and updated those events on the Switch class itself.

ghost assigned jacobheun Sep 26, 2018

ghost added the in progress label Sep 26, 2018

pgte reviewed Sep 27, 2018

View reviewed changes

vasco-santos suggested changes Sep 27, 2018

View reviewed changes

jacobheun added 15 commits October 2, 2018 21:35

feat: add basic state machine functionality to switch

c47ab13

fix: linting

a97c386

refactor: move connection.js to connection-manager.js

b4d1602

feat: add outgoing connection state machine

41e4a88

feat: functioning incoming connection fsm

5848e6d

fix: better support custom handlers

fix: linting

45ae65b

fix: stats

78203fd

docs: remove notes

0b9f5f1

test: bump circuit shutdown timeout

ef92ee2

fix: node 8 support

4b62917

feat: add class-is support for connections

7ab2151

refactor: clean up some logic and make inc muxed conns FSMs

674d55c

fix: cleanup todos, logic and event handlers

f4a1806

refactor: clean up logs

dd12ad1

feat: add dialFSM to the switch

56ea400

jacobheun force-pushed the feat/state-machine branch from 7a5d1b0 to 56ea400 Compare October 3, 2018 15:46

jacobheun added 4 commits October 3, 2018 18:01

refactor: rename test file

2e1f7b5

feat: add better support for closing connections

75f1370

test: add tests for some uncovered lines

0ee8157

refactor: do some cleanup

d921c2d

jacobheun added 2 commits October 5, 2018 17:16

feat: add additional fsm user support

6351365

feat: add warning emitter for muxer upgrade failed

3d469d0

jacobheun mentioned this pull request Oct 5, 2018

Make libp2p a state machine libp2p/js-libp2p#257

Merged

2 tasks

refactor: cleanup and add some tests

662a58f

jacobheun added 4 commits October 5, 2018 19:28

test: add test for failed muxer upgrade

730c2b6

test: add more error state tests for connectionfsm

6dd8f39

docs: update readme

a2e995e

docs: fix readme link

ef85443

daviddias reviewed Oct 17, 2018

View reviewed changes

jacobheun changed the title ~~[WIP] Feat/state machine~~ Make switch a state machine Oct 18, 2018

vasco-santos reviewed Oct 19, 2018

View reviewed changes

docs: clean up readme and jsdocs

8ff0375

vasco-santos approved these changes Oct 19, 2018

View reviewed changes

jacobheun merged commit 7dcabdd into master Oct 19, 2018

ghost removed the in progress label Oct 19, 2018

jacobheun deleted the feat/state-machine branch October 19, 2018 11:32

jacobheun mentioned this pull request Aug 22, 2019

[switch] Support multiple crypto modules libp2p/js-libp2p#437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make switch a state machine #278

Make switch a state machine #278

jacobheun commented Sep 26, 2018 •

edited

Loading

jacobheun commented Sep 26, 2018

pgte left a comment

pgte Sep 27, 2018

pgte Sep 27, 2018

jacobheun Sep 27, 2018

pgte Sep 28, 2018

jacobheun Sep 28, 2018

jacobheun Oct 3, 2018

pgte Oct 3, 2018

pgte Sep 27, 2018

jacobheun Sep 27, 2018

pgte Sep 28, 2018

vasco-santos left a comment

vasco-santos Sep 27, 2018

vasco-santos Sep 27, 2018

vasco-santos Sep 27, 2018

vasco-santos Sep 27, 2018

vasco-santos Sep 27, 2018

jacobheun commented Oct 4, 2018

daviddias Oct 17, 2018

jacobheun Oct 18, 2018

daviddias Oct 18, 2018

jacobheun Oct 18, 2018

jacobheun commented Oct 18, 2018

vasco-santos left a comment •

edited

Loading

vasco-santos Oct 19, 2018

vasco-santos Oct 19, 2018

vasco-santos Oct 19, 2018

jacobheun commented Oct 19, 2018

Make switch a state machine #278

Make switch a state machine #278

Conversation

jacobheun commented Sep 26, 2018 • edited Loading

Goal

Updates

TODOs

Thoughts on Future Additions

References

jacobheun commented Sep 26, 2018

pgte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun commented Oct 18, 2018

vasco-santos left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobheun commented Oct 19, 2018

jacobheun commented Sep 26, 2018 •

edited

Loading

vasco-santos left a comment •

edited

Loading