-
Notifications
You must be signed in to change notification settings - Fork 2
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
Suggestions to simplify Session management #5
Suggestions to simplify Session management #5
Conversation
103e73d
to
1dc77cf
Compare
1dc77cf
to
2ecc61f
Compare
src/handler/mod.rs
Outdated
|
||
// Decide whether to establish this connection based on our apettiite for unreachable | ||
if enr_not_reachable | ||
&& Some(self.sessions.tagged()) > self.nat_utils.unreachable_enr_limit |
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.
&& Some(self.sessions.tagged()) > self.nat_utils.unreachable_enr_limit | |
&& Some(self.sessions.tagged() + 1) > self.nat_utils.unreachable_enr_limit |
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.
Can this just be >=
?
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.
lgtm, not so good to clippy. left a some suggestions, if you commit those I'm happy with it.
} | ||
/// Determines if an ENR is reachable or not based on its assigned keys. | ||
pub fn is_enr_reachable(enr: &Enr) -> bool { | ||
enr.udp4_socket().is_some() || enr.udp6_socket().is_some() |
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.
nit:
enr.udp4_socket().is_some() || enr.udp6_socket().is_some() | |
enr.udp4_socket().or(enr.udp6_socket()).is_some() |
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.
Dont think we can do this, because the types are different. SocketAddrv4 vs SocketAddrv6
Ok. I think i've addressed everything here. Will continue with the main review :) |
I've made some changes which I think solve some bugs, simplify the code and make the diff smaller.
The main part of it was moving unreachable enr tracking to inside the LRUTimeCache in a generic way, by adding the concept of tagging. Elements can be optionally tagged when inserted into the cache. If we need to know how many tagged elements are in there we can just go like
cache.tagged()
.This allows us to easily track the number of unreachable sessions, just by checking tagged. There are a number of reasons why I think this is an improvement:
get_mut()
orlen()
on the cache was called. So elements could expire (time-wise) but the task will not get awoken until either of these functions are called. More importantly, the channel does not get populated whenremove()
is called. So in the NATHolePuncher tracking case, these were not being registered. I replaced this logic with a HashSetDelay, because it seemed we only cared about when elements were expiring. This is a true async struct and will fire exactly when things expire.remove()
function, they were not being counted as being removed from the tracker. I duplicated this logic in the nat_hole_puncher_tracker with a delay hashmap.In addition a few more changes I made:
Clone
andCopy
derive from Session and Keys. These are private keys and we don't want to copy them in memory for security reasons. For private keys, we try to keep one point in memory to store them and we zeroize that memory when it goes out of scope. I think the copy and clone were only needed for tests, from what I can tell.I think I have replicated all the logic however. If there's a mismatch let me know.