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

Investigate "handle socket error to prevent possible app crash, such as ECONNRESET" #113

Open
mklabs opened this issue Oct 27, 2016 · 9 comments

Comments

@mklabs
Copy link
Owner

mklabs commented Oct 27, 2016

See if we should rollback this commit to limit the described behavior in

303f95c#commitcomment-19591161

@creeperyang
Copy link
Contributor

creeperyang commented Oct 27, 2016

@matthewmueller I confirm that the error is caught by my pr.

However, different versions of node has different behavior about Error: read ECONNRESET (an error occured when client abruptly closed the connection of TCP.)

When I push the code, my console sometimes output the message:

Error: read ECONNRESET
    at exports._errnoException (util.js)
    at TCP.onread (net.js)

It's annoying and you can't control it. (Um, maybe you can listen to uncaughtexception).

I tested on [email protected], the ECONNRESET seems be dropped (wont output) defaultly?

By the way, maybe you can ignore it by filter the error with:

if (error.code !== 'ECONNRESET') { console.log(error) }

because the error is like

{ Error: read ECONNRESET
       at exports._errnoException (util.js:1026:11)
       at TCP.onread (net.js:569:26) code: 'ECONNRESET', errno: 'ECONNRESET', syscall: 'read' } }

@creeperyang
Copy link
Contributor

I don't think this will lead to tons of the error. When I test, one tab of chrome only lead to one error.

@mklabs
Copy link
Owner Author

mklabs commented Oct 27, 2016

Thanks @creeperyang for detailed explanation.

I need to test it further, but maybe we can silent out ECONNRESET type of errors

@creeperyang
Copy link
Contributor

creeperyang commented Oct 27, 2016

@mklabs yeah, you're right. It seems inevitable to get this error in our case.

@matthewmueller
Copy link

fwiw, this isn't an issue at all if i downgrade to "tiny-lr": "0.2.1"

creeperyang added a commit to creeperyang/tiny-lr that referenced this issue Oct 27, 2016
hemanth added a commit that referenced this issue Oct 28, 2016
ignore ECONNRESET socket error (see #113)
@aknuds1
Copy link
Contributor

aknuds1 commented Oct 28, 2016

I keep getting this error, Node 6.9.1.

@matthewmueller
Copy link

Could be wrong, but ignoring the error seems like a bandaid. If this error was inevitable, wouldn't it affect all websocket servers?

@asgoth
Copy link

asgoth commented Nov 14, 2016

Don't know if it is related, but with version 1.0.2 I sometimes get:

console - ... Uhoh. Got error undefined ...

Don't seem to get it with version 1.0.3 (for now). I was trying to check if something was changed in 1.0.3 which could explain it.

Only noticed a bug in one of the latest pr's: https://github.com/mklabs/tiny-lr/pull/114/files#diff-c945a46d13b34fcaff544d966cffcabaR157

Function apply expects an array, not an Error object (function call a comma separated list)

@creeperyang
Copy link
Contributor

creeperyang commented Nov 15, 2016

@asgoth yeah, a silly mistake. Really sorry for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants