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

Reduce latency and spikes by tunning TCP socket #137

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rafaeling
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 25.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 59.44%. Comparing base (d4f0ad3) to head (648a576).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
databroker/src/grpc/server.rs 25.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
- Coverage   59.47%   59.44%   -0.03%     
==========================================
  Files          34       34              
  Lines       15112    15121       +9     
==========================================
+ Hits         8988     8989       +1     
- Misses       6124     6132       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rafaeling rafaeling force-pushed the reduce_latency_by_tunning_tcp_socket branch 3 times, most recently from acf984f to e6868f6 Compare February 11, 2025 14:38
@rafaeling rafaeling force-pushed the reduce_latency_by_tunning_tcp_socket branch from e6868f6 to 0738a96 Compare February 11, 2025 14:40
.http2_keepalive_timeout(Some(Duration::from_secs(20)));
.http2_adaptive_window(Some(true))
.http2_keepalive_interval(None)
.http2_keepalive_timeout(None);
Copy link
Contributor

Choose a reason for hiding this comment

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

Really setting the timeout to None?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we set a timeout without keep-alive pings this could lead to closing an idle connection if there is nothing sent or received for the timeout specified time. Say we set .http2_keepalive_timeout(20s); and .http2_keepalive_interval(None) then after 20 s if there's no communication the connection will be closed. Ithink if we disable keep-alive we should set both to None. The Danger would be that we keep idle connections open for a very long time and they might be broken (e.g. we encounter broken connections only if we try to send something again).

This could possibly lead to ressource leaks as the server may still hold resources (like memory, file descriptors, etc.) associated with the connection and the request. If the server is not aware that the client has disconnected, it may continue to allocate resources for that connection, leading to resource leaks.

Maybe setting the keepalive timeout to 1 min? or even keep-alive interval to 1 min and timeout to 2 could be a solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the timeout is not relecant when the interval is None/Max (whoch seems t be the DEFAULT anyway accoridng to https://grpc.io/docs/guides/keepalive/ )

I would keep both at none, I thing resource "leak" is not an issue here. Linux kernel can have thousands of connections without issue, so mich more than you would "normally" expect in a vehicle. If talking about DDo/attacking it does not make a difference what we set here anyway, as an attacker would just create a lot of connections in a short timeframe.

@rafaeling rafaeling changed the title Reduce latency and spikes tunning by TCP socket Reduce latency and spikes by tunning TCP socket Feb 17, 2025
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.

4 participants