Skip to content

[BE] Gracefully handle redis disconnects - #198

Merged
liana-p merged 3 commits into
mainfrom
lp/redis-reconnect
Jan 21, 2022
Merged

[BE] Gracefully handle redis disconnects #198
liana-p merged 3 commits into
mainfrom
lp/redis-reconnect

Conversation

@liana-p

@liana-p liana-p commented Jan 20, 2022

Copy link
Copy Markdown
Contributor

🌟 What is the purpose of this PR?

Gracefully handle redis disconnects to not crash the app and let it reconnect.

🔍 What does this change?

  • Fixes an issue in a function that was called on a setInterval which would try to access an undefined object after a redis disconnect and crash the app
  • Added logging for various redis events so we get clear logs when redis errors/reconnects.

🔗 Related links

❓ How to test this?

Turn off the redis docker container while the app is running and see those events in the log, then turn it back on and see the app reconnects to redis, recovers and continues to work as expected.

@liana-p
liana-p requested a review from nathggns January 20, 2022 15:27
@github-actions github-actions Bot added area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) search-loader labels Jan 20, 2022
@semgrep-app

semgrep-app Bot commented Jan 20, 2022

Copy link
Copy Markdown

Semgrep was unable to comment inline on the following findings:

In packages/hash/realtime/src/index.ts:27-95, packages/hash/realtime/src/index.ts:27-82, packages/hash/realtime/src/index.ts:27-107:

Untrusted input concatinated with raw SQL query can result in SQL Injection.

Click a link below to tell your Security Team if this comment was:
👍 useful
👎 not useful (false positive)
🤔 confusing

Found by ajinabraham.njsscan.sql_injection.node_sqli_injection in Security Policy.

How do I suppress this?

@nathggns

Copy link
Copy Markdown
Contributor

I can confirm I've seen this issue and it's very annoying – thank you for aiming to fix it! Will take a look shortly.

@nathggns nathggns left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In a bad position in my local setup right now to review functionality, but the code looks great and it should solve the issue from what I can tell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants