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

chore: removed ping in favor of keepalive #86

Merged
merged 8 commits into from
Nov 14, 2024
Merged

Conversation

pubalokta
Copy link

@pubalokta pubalokta commented Nov 11, 2024

By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Description

We observed that limitd-redis fails to promptly detect a stale TCP connection to Redis, only realizing the connection is down after approximately 12 minutes. After this delay, it reconnects as expected.

This update enables the keepAlive feature in ioredis, which sets the KEEPALIVE option on the socket. This allows for a quicker reaction to stale connections.

Key changes:

  • The keepAlive feature is enabled by default and set to 10000 ms.
  • The keepAlive interval can be customized by passing the keepAlive argument to the LimitdRedis constructor.
  • Setting the keepAlive argument to a non-numeric value will disable the feature.
  • The manually-implemented ping mechanism against Redis has been removed, as we now rely on ioredis' keepAlive feature.

Testing

Describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.

Please include any manual steps for testing end-to-end or functionality not covered by unit/integration tests.

Also include details of the environment this PR was developed in (language/platform/browser version).

  • This change adds test coverage for new/changed/fixed functionality

  • Using the previous limitd-redis version:

Welcome to Node.js v18.20.2.
Type ".help" for more information.
> const LimitdRedis = require("limitd-redis");
undefined
> client = new LimitdRedis({uri:'localhost:6379', buckets:{}});
> process.pid
49290
$ lsof -a -p 49290 -i 4 -T f                               
COMMAND   PID       USER   FD   TYPE  DEVICE SIZE/OFF NODE NAME
node    49290       ****   25u  IPv4    ****      0t0  TCP localhost:58196->localhost:6379 (SO=KEEPALIVE=7215075...)
  • Using the new version - default value
Welcome to Node.js v18.20.2.
Type ".help" for more information.
> const LimitdRedis = require("limitd-redis");
undefined
> client = new LimitdRedis({uri:'localhost:6379', buckets:{}});
> process.pid
49290
lsof -a -p 51327 -i 4 -T f   
COMMAND   PID  USER   FD   TYPE  DEVICE SIZE/OFF NODE NAME
node    51327   ***   26u  IPv4     ***      0t0  TCP localhost:59435->localhost:6379 (SO=KEEPALIVE=10000...)
  • Using the new version - custom value
Welcome to Node.js v18.20.2.
Type ".help" for more information.
> const LimitdRedis = require("limitd-redis");
undefined
> client = new LimitdRedis({uri:'localhost:6379', buckets:{}, keepAlive: 5000});
> process.pid
54298
lsof -a -p 54298 -i 4 -T f
COMMAND   PID   USER   FD   TYPE  DEVICE SIZE/OFF NODE NAME
node    54298   ****   26u  IPv4    ****      0t0  TCP localhost:61643->localhost:6379 (SO=KEEPALIVE=5000....)

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not the default branch

@pubalokta pubalokta requested a review from a team as a code owner November 11, 2024 11:20
@pubalokta pubalokta force-pushed the enable-keepalive branch 2 times, most recently from f7bf8e6 to b14048c Compare November 11, 2024 16:34
@@ -10,6 +10,8 @@ on:

jobs:
test:
env:
CI: true
Copy link
Author

@pubalokta pubalokta Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added to be able to distinguish between local and CI, for customizing the environment if needed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not entirely sure if github doesn't add it themselves, but good anyway.

@pubalokta pubalokta merged commit 6c9ee68 into master Nov 14, 2024
7 checks passed
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

Successfully merging this pull request may close these issues.

2 participants