-
Notifications
You must be signed in to change notification settings - Fork 12
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
[PM-2752] Throttle incoming connections from the same ip #126
[PM-2752] Throttle incoming connections from the same ip #126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one!
@@ -150,7 +150,8 @@ class ScalanetModule(crossVersion: String) extends Module { | |||
ivy"org.scodec::scodec-core:1.11.7", | |||
ivy"org.bouncycastle:bcprov-jdk15on:1.64", | |||
ivy"org.bouncycastle:bcpkix-jdk15on:1.64", | |||
ivy"org.bouncycastle:bctls-jdk15on:1.64" | |||
ivy"org.bouncycastle:bctls-jdk15on:1.64", | |||
ivy"com.github.ben-manes.caffeine:caffeine:2.8.8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The projects we use Scalanet in already use guava
, which also has a timed cache. Shouldn't we stick to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm there is only some old versions of guava
- 21
, which is from 12-Jan-2017
, and also it exports only some wierd annotation library part - com.google.errorprone:error_prone_annotations
.
Taking those into account I would stick with caffeine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should upgrade then, there's version 30 from Dec 2020.
Looking at caffeine on Maven it has the same error_prone_annotations
dependency, there's even an issue for it, which references the guava issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it sounds like Caffeine is a bit better, like no extra thread for cleanups, so if you prefer it then 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure we can, I do not have strong preference here. As long it has putIfAbsent
and timed eviction it is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CacheBuilder has an example of expire after write, and apparently the asMap with putIfAbsent is still the way.
I also read that Caffeine handles async loading natively, whereas Guava is synchronous.
scalanet/src/io/iohk/scalanet/peergroup/dynamictls/CustomHandlers.scala
Outdated
Show resolved
Hide resolved
scalanet/src/io/iohk/scalanet/peergroup/dynamictls/CustomHandlers.scala
Outdated
Show resolved
Hide resolved
scalanet/src/io/iohk/scalanet/peergroup/dynamictls/CustomHandlers.scala
Outdated
Show resolved
Hide resolved
scalanet/src/io/iohk/scalanet/peergroup/dynamictls/CustomHandlers.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I'm sorry for suggesting a confusing name, see my comment about another option.
e3197c2
to
e8440f2
Compare
so ultimately lets go with |
Description
Create a possibility to throttle incoming connections from the same ip.
Some of design decisions:
maxSize
property to cache as it with typical throttling times (like lets say30s
) it would be really hard to use up all host memory. (Ip4 address takes 4bytes, so 1M of them would take only 4Mb + some overhead for java object fields)exipreAfterWrite
property. The similar effect could be accomplished by using java concurrent hash map, and scheduling clearing on netty eventLoops, but ultimately I think cache solution is clearer and worth bringing another dependency