libp2p.on() silently eats errors/exceptions

[mcclure]

Check out this libp2p with preact sample code to 0.29.0 demo (libp2p-preact branch). This is basically a rewrite of the libp2p-in-the-browser example. Run it with

npm install && npx webpack && (cd site && http-server -c-1)

Open this in a browser and you will see it makes lists of connected and discovered nodes. So far so good.

Okay, now edit the file src/index.tsx. Scroll down to line 163 and add a new first line of connectionManager.on(

node.connectionManager.on('peer:connect', (connection:any) => {
  let hfge:any         // New line, to make typescript happy
  hfge.waaf.oiew.fef() // New line

Obviously this is gibberish and will fail. But now try running the program. You will find

1. The program fails uncleanly; although the problem is Javascript throws an exception when you use the undefined "hfge" object, the visible symptom will be that it appears we only discover and never connect to them.
2. No error of any kind is printed in the Javascript console (tested on Chrome).

Impact

I would consider this a medium to high problem with the API. It is a problem because silent failures make it very difficult to debug what is failing or even realize something is failing. Exacerbating the problem is

1. it's really unclear to me what the type is of the connectionManager.on() object, what the type of the object passed to it is, or what the methods on it are— if this is documented anywhere I've not found it in the many months, they are just magic functions in the getting started examples
2. The signatures of these objects/functions routinely change from release to release. I got stuck on this because I was upgrading my code from 0.27.x to 0.29.x, the holdover code was bad because it used the old method signatures, and the lack of an error caused me to mis-diagnose the problem (ie: I thought the problem was that I was discovering peers but not connecting, but it was a simpler problem that I was connecting to peers but my on-connect handler was crashing).

This makes it unusually likely that users will make trivial errors in these exact functions that node.on will not catch.

Template questions

Version: 0.29.0, as documented in example repo package.lock
uname -a of test system: Darwin Pearl.local 17.7.0 Darwin Kernel Version 17.7.0: Sun Dec 1 19:19:56 PST 2019; root:xnu-4570.71.63~1/RELEASE_X86_64 x86_64
Test version of chrome: 85.0.4183.83 (Official Build) (64-bit)

[vasco-santos]

Hey @mcclure

the visible symptom will be that it appears we only discover and never connect to them.

It discover and connects, but the event handler for informing of the connect fails to execute.

The signatures of these objects/functions routinely change from release to release. I got stuck on this because I was upgrading my code from 0.27.x to 0.29.x, the holdover code was bad because it used the old method signatures, and the lack of an error caused me to mis-diagnose the problem (ie: I thought the problem was that I was discovering peers but not connecting, but it was a simpler problem that I was connecting to peers but my on-connect handler was crashing).

As written in #750 we have the migration guides to help migrating. We want to minimize breaking changes, but but this project is still in Alpha, lots of development is happening and sometimes we need to break things to move to better approaches/APIs. The best I can recommend you it to check the migration guides, as these changes are described there per release.

This makes it unusually likely that users will make trivial errors in these exact functions that node.on will not catch.

Regarding this case, this is how Node.js Event Emitter handles errors (it does not!). They only provide this hint. However, I agree that this silent error is bad and might create issues for users. We can try to minimize this by checking other Event Emitter libs or try to provide a general error event. What do you think on this @jacobheun ?

[vasco-santos]

I have looked into this. In the connectionManager events, the error is being absorved behind the scenes as it is wrapped on a promise that comes from upgrading the connection. The upgrader calls the onConnection handler, which will call the connectionManager onConnect which does the emit. The emit triggers the handler that causes the error, but it is just ignored in the browser.

In node this is not an issue because "the error is forwarded to the process.on('error') event on process.nextTick()" here. This does not happen in the most used shims for browsers. I have checked the used shims for webpack and parcel.

The best solution according to the EventEmitter docs is to throw all the side effects of the emit in the global context like node does. Node uses process.nextTick, which we cannot use in the browser without a shim. The easiest solution is to wrap the this.emit() with a try ... catch and throw the error using setTimeout.

Also, we should check if the other top level emitted events have similar issues

cc @jacobheun

[maschad]

Closing due to staleness