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

Limit size of outbound write buffer #127

Closed
wants to merge 1 commit into from

Conversation

ajsutton
Copy link
Contributor

Avoids excessive memory usage when a peer requests data faster than it can read it. There is probably a need to make the values used configurable and possibly vary them based on the stream state (ie multi stream negotiation should have little to no write buffer beyond what the OS provides, but things like Eth2 request for blocks might need more). Not entirely sure how that should fit into the API for jvm-libp2p though.

Changes made:

  • Disable autoread when the channel become unwritable. This ensures we stop processing incoming requests when responses can't be read anyway.
  • Adds a WriteTimeoutHandler to fail writes if the buffer remains full for 30 seconds. Auto-close then causes these connections to be closed.
  • Limit size of write buffer to 1k instead of 64k. Ensures these limits apply quickly.

Prior to these changes a server handling a constant stream of ls multistream requests where the responses are not read, would use 5-6Gb of resident memory and all available CPU. Afterwards resident memory only grows by a few Mb, the CPU usage is negligible and the attacking connection is disconnected.

…ing messages we can't send the response for anyway.

Disconnect if the write buffer remains full for 30 seconds.
Limit size of write buffer to 1k instead of 64k.
@Nashatyrev
Copy link
Collaborator

Interesting tweaks 👍
The only thing I'm concerned about is pushing all these tweaks by default, especially WriteTimeoutHandler. Let me think a bit on how to make this configurable

@ajsutton
Copy link
Contributor Author

Yeah agreed - maybe they are defaults, though I'd want to spend more time making sure the number really do make sense, but they really do need to be configurable. I spent a bunch of time trying to find a way to just apply these settings within Teku without having to change jvm-libp2p but couldn't find a way to get hold of the netty classes. It does feel like having DOS protections like this fairly easy to apply would be useful.

May also be worth looking at what other libp2p implementations do out of the box for this kind of thing too.

@Nashatyrev
Copy link
Collaborator

Right now I'm thinking on adding some kind of 'advanced option': connectionPreHandler which would be invoked right after connection establishing. That would be the right place to add your tweaks on a client side.
That would also be the right place to add a 'firewall' either basic libp2p (which I'm planning to create) or any other custom one.
Does it sound reasonable?

@Nashatyrev
Copy link
Collaborator

Nashatyrev commented Jul 29, 2020

@ajsutton what do you think on implementing this on Teku side: Consensys/teku#2478 ?
builder.getDebug().getBeforeSecureHandler() is probably not the most user-friendly place. I would adjust the Libp2p Builder later

@ajsutton
Copy link
Contributor Author

Yep that sounds good to me.

@ajsutton ajsutton closed this Jul 29, 2020
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.

None yet

2 participants