-
-
Notifications
You must be signed in to change notification settings - Fork 619
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
ECONNRESET #128
Comments
Can you post very basic example of how are you using pool? Pool code in node-mysql2 is exactly the same as in node-mysql, but couple of month behind (I pull it manually from time to time) |
Yes, of course:
A sample of how one might use the above code:
|
System:
|
Thanks for examples. So I guess expected behaviour is error parameter in the callback (propagated up as exception via var db = require('./db.js');
var result;
try {
// we should be either here
result = yield db.query('SELECT 1;');
} catch(e) {
// or here regardless on connection
console.log(e);
} This might be a bug similar to mysqljs/mysql#941 - I'll try to update to latest pool code from node-mysql |
Yes, I have a try/catch on all of my queries. It doesn't get caught. I guess it might be related to mysqljs/mysql#941 |
The |
In that issue, the error was given back through the generic callback, just not emitted (unless I read it too fast). After reviewing some of the code in this package (also pretty quickly), it seems that |
Yes, the error was given through the callback, but if you didn't supply the callback (because you wanted to use events), then the error would never appear anywhere. That was what the issue was about :) |
Hi, Just wanted to update and say that I still get this issue, I'm not sure why though. I'm not able to replicate. Here is a longer log from Heroku/Papertrail:
As you can see, I get Any suggestions? |
I get this error too but only when I use koa-proxy which depends on co-request. When I hit the post directly on the (VPN) server, I don't have the problem.
The code I'm using is very simple
|
@jsbinette - is that new connection can't be established to mysql server or existing drops? Do you have local mysql server localhost:3306 ? Which version of node-mysql2 are you using? |
@sidorares Sorry turns out it doesn't apply. In my case, koa-better-body interfered somehow with the post when it went thru the proxy. |
getting this sometimes randomly. simple test server, nothing complex happening on it.
|
@farzher could be anything - server idle timeout, network issues etc |
@sidorares you don't seem surprised that this causes random errors on production servers. |
@farzher this is a low level driver and retries out of the scope. It's not clear from your report if you can catch and process error in your code or it's something that your code can't handle ( if latter, it definitely need to be fixed ) The "best practise" usually is to never use individual connections directly and use connection pool ( even if one connection is enough from perf point of view ). But even with pool you absolutely must be able to handle errors from production code and make a decision what to do ( retry / log / report to user ) |
For one I agree that this driver should have a ‘keep alive’ option. It’s hard to debug and there are no ‘using’ keyword like c# that makes it easy o handle in JavaScript
Thursday, January 10, 2019, 6:42 PM -0500 from [email protected] <[email protected]>:
…
@farzher this is a low level driver and retries out of the scope. It's not clear from your report if you can catch and process error in your code or it's something that your code can't handle ( if latter, it definitely need to be fixed )
The "best practise" usually is to never use individual connections directly and use connection pool ( even if one connection is enough from perf point of view ). But even with pool you absolutely must be able to handle errors from production code and make a decision what to do ( retry / log / report to user )
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub , or mute the thread .
|
@jsbinette you can always use pool instead of single connection for that connection here is "immutable" in that sense. If it dies you create new one instead of ressurrecting internal state (re-connecting tcp / re-doing hanshake etc ) |
@sidorares that's fair. maybe make mention that in the readme so lazy people like me are reminded to do proper error handling. i'm using a pool, you can see the error is |
error handling is always required with any library. Built in retries just make it less frequent/visible ( you still have to handle case when number of attempts is over limit ) In theory we might have it easier for pool users to have automatic retries, for example have a function you pass to config that can decide "retry or not" based on failed query. |
i guess i didn't mean error handling, error.. retrying? idk it's not a concept i thought about. if so you're telling me i'm supposed to look at the error message you return, figure out which ones are retryable, and retry? that's the part i'm lazy about, i assumed you'd take care of that because why should i have to. |
The biggest problem is the driver doesn't know what's save to retyr ( aka "idempotent" ). Say you do "please delete row with oldest updatedAt" and you get network error. At the moment you see error you don't even know if row was already deleted, so recovery from error is completely app logic specific |
makes sense. i was just running a select though, you could always auto retry that. you're saying most of the time if the connection is messed up before you try to execute the query, you'll auto retry to get a working connection? but this ECONNRESET error i got happened at an unlucky time where you weren't sure if it executed? |
driver sets Line 146 in efec9d5
|
for anyone getting this error randomly. i guess you're supposed to write this wrapper around function query_with_retries(sql, params, retries_left=1) {
try {
return pool.query(sql, params)
} catch(err) {
if(retries_left >= 1 && err.code === 'ECONNRESET') {
console.error({msg:'query_with_retries retrying!', retries_left, err})
return query_with_retries(sql, params, retries_left-1)
} else {
throw err
}
}
} |
I'm sorry to wake the dead here, but I'm struggling to fix this in my code as well. It has caught me by surprise after deploying to production, which isn't the ideal time to encounter the error. MySQL has a I see this in the MySQL documentation, but it doesn't seem to be the case for mysql2:
Thanks for the support. |
Very late to the discussion, but @benbotto could you please tell me how you solved this issue finally? I wasn't able to find any proper solutions on the internet. |
I get these errors sometimes. Randomly.
and
I read somewhere that this also happens on
node-mysql
, but that it could be fixed by using pool. I began using pool, but it still happens.Suggestions?
The text was updated successfully, but these errors were encountered: