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

remove request throttlers #1233

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

Conversation

larskuhtz
Copy link
Contributor

@larskuhtz larskuhtz commented May 18, 2021

From a brief inspection of the code it seems that the request throttling middleware uses a possibly inefficient underlying cache implementation. The use of STM may be inefficient under contention (although, probably only a problem when a large number of new peers is added in a short amount of time). There is also a tight loop during initialization of new cache items (should that be wrapped into STM retry?).

It is not clear if cache expiration is implemented correctly. It seems that items always expire after a fixed amount of time and the lease isn't renewed if they are queried in-between. With such a behavior 1. items are deleted only when used, 2. the cache would be reinitialized regularly even for keys that are used regularly. With that the current expiration of 5 seconds seems much too short.

It seems that the throttling middleware never purges the cache, which means that it would leak memory (luckily the leak is limited by the number of peers that existed in the network since the node was started).

Overall, throttling of the service API shouldn't be done within chainweb node, but should rather be delegated to a reverse proxy. Throttling of the P2P API should, if needed be implemented in a more efficient way, and should also include throttling of the overall number of open connections.

So, the following should be fixed:

  • increase cache timeout for throttling keys (at least > 300). The overhead per bucket is just a few words.
  • implement frequent cache purges to eliminate old keys
  • fix the tight loop in retrieveCache for CacheStateInitializing by adding STM retry.
  • remove throttling for non-P2P endpoints.
  • consider adding an overall (all addresses) 503 throttle for the P2P endpoints. Alternatively, consider use of iptables to limit total number connections on the bootstraps. However, be careful and explore how this affects the availablility of the nodes of bootstrapping purposes.

@larskuhtz larskuhtz requested a review from mightybyte May 18, 2021 03:33
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.

1 participant