-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Add delay option for socket.setKeepAlive #1396
Conversation
This is critical as because of this issue: nodejs/node#24689 though at this stage cannot sure if it is node issue |
I'm going to go ahead and merge, but I think we should investigate the root cause here as we might need a default that isn't 0. |
@stockholmux I strongly agree to have a value which is not the default 0. I can make the change if this sounds OK, also may i know when we might get the new version? |
I consulted with some colleagues on this. I don't think there is actually a rational/universal initial delay default. I'm ok with this as-is. |
Nice. no worries, as long as this already provides an interface to whom, like us, facing the issue, to change the setting. Thank you @stockholmux |
Hi @betimer ! |
@anhzhi I found that idle redis connection will be disconnected every 2 hours, it is strange. It is solved by using your code: client.on('connect', function () {
var socket = client.stream
socket.setKeepAlive(true, 30 * 1000)
}) |
This would work, but it is an ugly workaround. There is a special option for this, the package just need to be updated on npm. We have switched to |
Pull Request check-list
npm test
pass with this change (including linting)?Description of change
Add option for socket_initialdelay when calling socket.setKeepAlive from node API