-
Notifications
You must be signed in to change notification settings - Fork 270
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(connlib): remove outdated assertion due to possibility of network changes #4222
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Terraform Cloud Plan Output
|
… changes and drain events after refreshing allocations on reconnect
cd4446c
to
45fd085
Compare
rust/connlib/snownet/src/node.rs
Outdated
@@ -144,6 +144,8 @@ where | |||
for allocation in self.allocations.values_mut() { | |||
allocation.refresh(now); | |||
} | |||
|
|||
self.bindings_and_allocations_drain_events(); |
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.
This is actually wrong. refresh
is async so we don't have any events here. But we should add this function to the top of poll_event
to ensure we drain them in time.
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.
Should I do it with this PR?
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.
If you want to. I just don't want to add this here because it is pointless :)
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.
added :)
Performance Test ResultsTCP
UDP
|
@conectado That seems to break the snownet tests as those are very strict on what gets emitted when. I reckon just remove it for now. We might remove those tests soon and I don't want to block this on it. Also, your PR title needs to be updated to match the implementation of the PR. |
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.
ACK modulo CI
19a6a88
to
3537718
Compare
found while working on #4163