search

ssbc / multiserver

A single interface that can work with multiple protocols, and multiple transforms of those protocols (eg, security layer)
MIT License
104 stars 28 forks source link

fix net plugin to propagate listen() errors up #51

Closed staltz closed 5 years ago

staltz commented 5 years ago

I get a lot of reports that Manyverse is crashing, and the errors look like this:

Error: listen EINVAL fe80::c376:4a3f:a128:3466:26831
    at Server.setupListenHandle [as _listen2] (net.js:1269:19)
    at listenInCluster (net.js:1334:12)
    at doListen (net.js:1460:7)
    at process._tickCallback (internal/process/next_tick.js:63:19)

The correct fix for this error (has to do with suffixing the scope_id e.g. %wlan0 to the IPv6 address) is not in this PR, but anyway, I found that this error is coming from the server.listen call in multiserver/plugins/net and was surprised that there was no handling of server errors, so this PR adds that, by simply detecting that error and sending it upwards to the startedCb callback, which can belong to the combined multiserver.

arj03 commented 5 years ago

Looks good to me

staltz commented 5 years ago

Pinging other people who could help review this: @Happy0 @dominictarr @regular

dominictarr commented 5 years ago

will the server ever emit errors other than when you are trying to call listen?

staltz commented 5 years ago

@dominictarr I updated the PR to address what you mentioned. I think this PR is now good to merge, but we still would need to talk about having a server.on('error', handler) for all moments post-initialization, because otherwise this will cause crashes in Manyverse. That is, this PR won't protect us from EINVAL crashes in Manyverse, and I'm still rolling out a fork of multiserver instead.

staltz commented 5 years ago

Actually, correction: my fork of multiserver does not fix the crash, unfortunately. The crash is being tracked also here in nodejs-mobile.

Regardless, this PR seems good to merge.

cryptix commented 5 years ago

@regular also reported and fixed this here: https://github.com/ssbc/multiserver-scopes/pull/2

regular commented 5 years ago

@cryptix no, my PR fixes a cause of an error, this one has a wider scope, adding the ability to handle errors. Bay of plenty, like all Electron apps, I assume, also suffered from this non-handling of listener errors, because that causes an uncaught exception which by default blows up right into the user's face.

dominictarr commented 5 years ago

merged into 3.6.0

please don't wait for me to merge stuff, this is a pretty straight forward thing, someone please make the call and merge it