Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection reset error stops spark after some time? #23

Closed
rustyrussell opened this issue Sep 11, 2018 · 13 comments
Closed

Connection reset error stops spark after some time? #23

rustyrussell opened this issue Sep 11, 2018 · 13 comments
Labels
bug Something isn't working

Comments

@rustyrussell
Copy link

From my logs (stdout & stderr of spark). This has happened several times. spark-wallet --version == 0.1.1.

...
GET /stream?access-key=RAviKLwv5ZZBLUDngcBiagM7cLdPv923iSy6MdrVdus 200 25.565 ms - -
POST /rpc 200 13.431 ms - 411
uncaughtException, stopping process
Error: read ECONNRESET
    at _errnoException (util.js:1022:11)
    at TCP.onread (net.js:628:25)
@shesek shesek added the bug Something isn't working label Sep 11, 2018
@shesek shesek closed this as completed in 81fdabd Sep 11, 2018
@shesek shesek reopened this Sep 11, 2018
@shesek
Copy link
Owner

shesek commented Sep 11, 2018

I believe 81fdabd should fix this, can you try running from master and confirm?

$ git clone https://github.com/shesek/spark-wallet && cd spark-wallet && npm install && npm run dist:npm
$ ./dist/cli.js [args]

@shesek
Copy link
Owner

shesek commented Sep 11, 2018

Hmm, re-reading the error log, this might not be it (but 81fdabd is desirable regardless). Kinda hard to diagnose without a local reproduction, I'll ponder on this some more :)

@ghost
Copy link

ghost commented Sep 11, 2018

I got the same error almost immediately after the first or second payment:

GET /stream?access-key=S5f2ZBtPZ4UdGEuGZSvKaSDMyVltP236KPRTdqNGQ ^[[32m200 ^[[0m3.091 ms - -
GET /stream?access-key=S5f2ZBtPZ4UdGEuGZSvKaSDMyVltP236KPRTdqNGQ ^[[32m200 ^[[0m1.676 ms - -
uncaughtException, stopping process
Error: read ECONNRESET
    at _errnoException (util.js:992:11)
    at TCP.onread (net.js:618:25)

Did a "git pull" and "npm run dist:npm" and the error didn't show up again after a few payments..

@shesek
Copy link
Owner

shesek commented Sep 11, 2018

almost immediately after the first or second payment:

Is this after an incoming or an outgoing payment?

Did a "git pull" and "npm run dist:npm" and the error didn't show up again after a few payments..

Thanks for checking this! Please report back if you're bumping into this error again.

@ghost
Copy link

ghost commented Sep 11, 2018

They were all outgoing payments, also the few that crashed it before the update. Did 2 outgoing payments extra "a couple of hours ago" (as the wallet tells me). The wallet keeps running without any problems but I'll report back if it occurs again.

@rustyrussell
Copy link
Author

Nope, no help :(

POST /rpc 200 22.104 ms - 15
POST /rpc 200 17.722 ms - 411
uncaughtException, stopping process
Error: read ECONNRESET
    at _errnoException (util.js:1022:11)
    at TCP.onread (net.js:628:25)

@ghost
Copy link

ghost commented Sep 12, 2018

I saw no problems at all sending payments, so I tried receiving a payment from coinpanic.com and Spark crashed:

POST /rpc ^[[32m200 ^[[0m52.560 ms - -
POST /rpc ^[[32m200 ^[[0m17.866 ms - -
GET /stream?access-key=S5f2ZBtPZ4UdGEuGZSvKaSDMyVltP236KPRTdqNGQ ^[[32m200 1.382 ms - -
POST /rpc ^[[32m200 ^[[0m152.606 ms - -
GET /stream?access-key=S5f2ZBtPZ4UdGEuGZSvKaSDMyVltP236KPRTdqNGQ ^[[32m200 48.351 ms - -
POST /rpc ^[[32m200 ^[[0m271.628 ms - -
uncaughtException, stopping process
Error: read ECONNRESET
    at _errnoException (util.js:992:11)
    at TCP.onread (net.js:618:25)

EDIT: Today I made and received 2 payments and Spark had no problems. So it's not that Spark crashes all the time.

@darosior
Copy link

Hi, maybe this information could help you : nmap -Pn the ip shuts down Spark every time with the same error.

@shesek
Copy link
Owner

shesek commented Sep 23, 2018

@darosior Thanks! That's very helpful, I am able to reproduce with that.

It looks like this is related to the changes introduced in 62798ca. Removing the use of createMultiServer appears to fix the issue:

diff --git a/src/transport/tls.js b/src/transport/tls.js
index 3b8f1e5..874dc18 100644
--- a/src/transport/tls.js
+++ b/src/transport/tls.js
@@ -10,7 +11,8 @@ const defaultDir = path.join(require('os').homedir(), '.spark-wallet', 'tls')
 
 module.exports = (app, name=app.settings.host, dir=defaultDir, leEmail) => {
   const tlsOpt = leEmail ? letsencrypt(name, dir, leEmail) : selfsigned(name, dir)
       , redir  = (req, res) => (res.writeHead(301, { Location: `https://${ req.headers.host || name }/` }), res.end())
-      , server = createMultiServer(tlsOpt, app, redir)
+      , server = https.createServer(tlsOpt, app)

I knew from the beginning that createMultiServer is quite hacky and error-prone, and it has already caused issues previously (#10), so I think I'm just going to remove it. This means that users trying to access a TLS-enabled Spark server without using https:// will receive a connection error (instead of a 301 redirection), but I think that might be better than continuing to maintain this hack. :-\

@shesek
Copy link
Owner

shesek commented Sep 25, 2018

Actually... it looks like createMultiServer is not to blame here. The cause is much simpler: the lower-level net module TCP server doesn't appear to handle error events by default (like the higher-level http(s) modules do), which causes any socket error to raise an uncaught exception and crash the process. These errors should be treated as warnings rather than being fatal.

The simple fix is to explicitly bind for error events, log them to STDERR, and let the process keep running. This was implemented in 56635b8. I will soon release v0.1.2 with this fix.

I guess createMultiServer gets to live another day. :)

shesek added a commit that referenced this issue Sep 25, 2018
It appears that unlike `http(s)` servers, plain `net` module servers don't handle
the socket `error` event, which causes an uncaught exception and stops the process.

With this change, we explicitly handle the error, log it to stderr
and keep the process running.

Refs #23.
@shesek
Copy link
Owner

shesek commented Sep 25, 2018

Released in v0.1.2. @darosior @sumBTC @rustyrussell could you please confirm this fixes it?

@ghost
Copy link

ghost commented Sep 25, 2018

My conclusion is that the issue has been solved. Tried all kinds of stuff and sometimes I see a connection warning on the phone but the process on the server keeps going. Nice and thanks a lot!

@shesek
Copy link
Owner

shesek commented Sep 30, 2018

Alright, I'm closing this. Please report back if you're still experiencing crashes.

@shesek shesek closed this as completed Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants