-
Notifications
You must be signed in to change notification settings - Fork 13
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 Timeout #23
Comments
|
Thanks |
Ah - |
Maybe just to PING/PONG every X minutes?
пт, 21 июн. 2019 г., 17:50 Chris Shucksmith <notifications@github.com>:
… Ah - redisConnectWithTimeout() doesn't use libuv, it's a plain old
blocking/synchronous API in hiredis, exactly as this reporter figured out
too redis/hiredis#290 <redis/hiredis#290>
So it's not that easy, since libuv has no built in api for adding a
timeout to an async socket connection attempt. There is a 3rd party
workaround on the libuv help issue list libuv/help#54
<libuv/help#54> this would need tidying up and
incorporating into hiredis first before we could use it.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#23?email_source=notifications&email_token=AAGFYGZYC6YMY6V27F42DALP3TTB3A5CNFSM4FPCMYU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYIVXVA#issuecomment-504454100>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGFYGZUNOT2HRJ63RMGYA3P3TTB3ANCNFSM4FPCMYUQ>
.
|
Would be great to have a feature to set a
connectTimeout
and have the native addon give up on connecting after that time. In the wild I've seen a complete hang after the first reconnect from which this adapter never recovers. Unfortunately I can't reproduce but it seems to happen if redis crashes unexpectedly and doesn't tear down the socket correctly.I've implemented it partially on the JS side but this doesn't seem like the right approach - if the remote is simply slow to connect, the
onConnect
callback from the lagging connection will fire and you might get multipleconnect
events. So it seems best to do this in the native addon.The text was updated successfully, but these errors were encountered: