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

fix: Add limits to LiveQueryStore to prevent high memory usage #4893

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

dima74
Copy link
Contributor

@dima74 dima74 commented Jul 25, 2024

Description

Added two new config parameters which limits number of live queries (global and per-user). Default value for both is 128. This is needed to prevent high memory usage in cases when client sends a lot of queries with large output, but do not fully consume those queries (that is obtain only first batch and doesn't send requests for the next batches). Normally clients should not do this, but malicious third-party can send such requests to DDOS iroha peer. In case limits are exceeded, iroha peer will return error "429 too many requests" for the query request.

  • Names of parameters:
[torii]
query_store_capacity = 128
query_store_capacity_per_user = 128

Also:

  • Changed default value of query idle time from 30s to 10s (for the same reason to decrease potential peak memory usage)
  • Changed default value of fetch size from 10 to 100 (I think it is strange that when query output has e.g. 1000 items, client currently has to send 100 requests to get full query result)

Some notes about implementation:

  • Currently when adding new query, the message is send to mpsc::channel, and LiveQueryStore thread does the addition. I changed it to perform addition directly without mpsc::channel since we now need to check capacity and return error to client if it exceeds.

Linked issue

Closes #4830

Benefits

Add limit to number of live queries to prevent high memory usage in cases when client don't fully consume query output.

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@dima74 dima74 self-assigned this Jul 25, 2024
@github-actions github-actions bot added api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha labels Jul 25, 2024
Copy link

@BAStos525

@dima74 dima74 force-pushed the diralik/live-query-store-limit branch from 83fdfc5 to d2620ed Compare July 26, 2024 07:35
mversic
mversic previously approved these changes Jul 26, 2024
@mversic
Copy link
Contributor

mversic commented Jul 26, 2024

@dima74 with_coverage is failing if you haven't noticed

@mversic mversic force-pushed the diralik/live-query-store-limit branch from 84381f8 to d44b748 Compare July 26, 2024 08:13
@mversic mversic self-assigned this Jul 26, 2024
@mversic mversic enabled auto-merge (squash) July 26, 2024 08:18
@mversic mversic merged commit c529992 into hyperledger-iroha:main Jul 26, 2024
12 of 13 checks passed
@dima74 dima74 deleted the diralik/live-query-store-limit branch July 26, 2024 09:03
@DCNick3 DCNick3 removed their assignment Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate how Iroha operates under load under Ubuntu/Debian
4 participants