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.

Sign up for GitHub

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

Jump to bottom

TTL for WS Connections #1011

Merged
merged 8 commits into from
Apr 4, 2023
Merged

TTL for WS Connections #1011

merged 8 commits into from
Apr 4, 2023

Conversation

AlfredoG87
Copy link
Collaborator

@AlfredoG87 AlfredoG87 commented Mar 24, 2023 •
edited
Loading

Description:
Adding TTL to websocket connections to avoid keeping connections for too long.

Related issue(s):

Fixes #986

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (eba1350) 74.63% compared to head (9ae0ee1) 74.63%.

❗ Current head 9ae0ee1 differs from pull request most recent head fb85d75. Consider uploading reports for the commit fb85d75 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1011   +/-   ##
=======================================
  Coverage   74.63%   74.63%           
=======================================
  Files          30       30           
  Lines        2030     2030           
  Branches      386      386           
=======================================
  Hits         1515     1515           
  Misses        410      410           
  Partials      105      105

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@AlfredoG87 AlfredoG87 self-assigned this Mar 24, 2023
@AlfredoG87 AlfredoG87 added this to the 0.21.0 milestone Mar 24, 2023
@AlfredoG87 AlfredoG87 marked this pull request as ready for review March 24, 2023 16:31
Nana-EC
Nana-EC requested changes Mar 24, 2023
View reviewed changes
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG.
Missing documentation of new config value

packages/ws-server/src/webSocketServer.ts Outdated Show resolved Hide resolved
packages/server/tests/acceptance/ws/subscribe.spec.ts Outdated Show resolved Hide resolved
@Nana-EC Nana-EC added enhancement New feature or request P2 labels Mar 28, 2023
Nana-EC
Nana-EC requested changes Mar 28, 2023
View reviewed changes
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

nice, almost there.
Question on a test and decrement logic

.env.example Outdated Show resolved Hide resolved
packages/server/tests/acceptance/ws/subscribe.spec.ts Outdated Show resolved Hide resolved
packages/ws-server/src/webSocketServer.ts Outdated Show resolved Hide resolved
Nana-EC
Nana-EC previously approved these changes Mar 31, 2023
View reviewed changes
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG, with comment suggestion for test clarity

packages/server/tests/acceptance/ws/subscribe.spec.ts Show resolved Hide resolved
@AlfredoG87
Adding TTL to websocket connections to avoid keeping connections for …
042e8e4 
…too long.

Signed-off-by: Alfredo Gutierrez <alfredo@swirldslabs.com>
@AlfredoG87
Adding configuration documentation for
edef0f5 
WS_MAX_CONNECTION_TTL and other WS configuration values.
Addressing some suggestions from PR reviews.
Signed-off-by: Alfredo Gutierrez <alfredo@swirldslabs.com>
@AlfredoG87
Adding configuration documentation for SUBSCRIPTIONS_ENABLED
6cd6176 
Signed-off-by: Alfredo Gutierrez <alfredo@swirldslabs.com>
@AlfredoG87
Returning assigment that probably was removed during rebase by accident
2196322 
Signed-off-by: Alfredo Gutierrez <alfredo@swirldslabs.com>
@AlfredoG87
Including Fix of PR-1017 so acceptance tests pass.
fe2bab0 
Signed-off-by: Alfredo Gutierrez <alfredo@swirldslabs.com>
@AlfredoG87
improve tests readability and remove fix from other PR to avoid tests…
498c3e1 
… fail, avoided test failed by moving my test above the last one

Signed-off-by: Alfredo Gutierrez <alfredo@swirldslabs.com>
@AlfredoG87
Improved test by reading the property WS_MAX_CONNECTION_TTL when the …
f006d81 
…connection is created, instead than when the webSocketServer is instantiated, this allows for better testability.

Also added comments to tests for better readability.

Signed-off-by: Alfredo Gutierrez <alfredo@swirldslabs.com>
Nana-EC
Nana-EC previously approved these changes Apr 3, 2023
View reviewed changes
packages/server/tests/acceptance/ws/subscribe.spec.ts Outdated
@@ -104,6 +106,11 @@ describe('@web-socket Acceptance Tests', async function() {
wsProvider.destroy();
});

this.afterAll(async () => {
// Return ENV variables to their original values
process.env.WS_MAX_CONNECTION_TTL = '300000';
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: if this is supposed to be the original value you should probably cache it first.
The current logic will require this value get updates should the default WS_MAX_CONNECTION_TTL ever change.

@AlfredoG87
cache WS_MAX_CONNECTION_TTL so when restored used env value instead o…
fb85d75 
…f initial default

Signed-off-by: Alfredo Gutierrez <alfredo@swirldslabs.com>
@sonarcloud
Copy link

sonarcloud bot commented Apr 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

lukelee-sl
lukelee-sl approved these changes Apr 4, 2023
View reviewed changes
Copy link
Member

@lukelee-sl lukelee-sl left a comment

Choose a reason for hiding this comment

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

LGTM

@AlfredoG87 AlfredoG87 requested a review from Nana-EC April 4, 2023 01:28
Ivo-Yankov
Ivo-Yankov approved these changes Apr 4, 2023
View reviewed changes
Copy link
Collaborator

@Ivo-Yankov Ivo-Yankov left a comment

Choose a reason for hiding this comment

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

LGTM

Nana-EC
Nana-EC approved these changes Apr 4, 2023
View reviewed changes
Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LGTM

@AlfredoG87 AlfredoG87 merged commit 66753ae into main Apr 4, 2023
@AlfredoG87 AlfredoG87 deleted the 986-TTL-ws-connections-ttl branch April 4, 2023 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ivo-Yankov Ivo-Yankov Ivo-Yankov approved these changes

@Nana-EC Nana-EC Nana-EC approved these changes

@lukelee-sl lukelee-sl lukelee-sl approved these changes

@ebadiere ebadiere Awaiting requested review from ebadiere ebadiere is a code owner

Assignees

@AlfredoG87 AlfredoG87

Labels
enhancement New feature or request eth_subscribe P2
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.

WS Server should have a TTL for open connections
5 participants
@AlfredoG87 @codecov-commenter @Ivo-Yankov @Nana-EC @lukelee-sl