-
Notifications
You must be signed in to change notification settings - Fork 269
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
refactor(connlib): use events for pushing updated resource list #5035
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Performance Test ResultsTCP
UDP
|
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.
I tried it on Windows and it gets the resources fine.
Getting rid of the callbacks eventually sounds great.
I left a comment asking if we could even remove the resource list from the buffered events, since the Clients (AFAIK) will never care about anything but the most recent resource list.
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,
Though wrt the description, handle_timeout
changes are internal to connlib, it didn't expose any of the changes to the external API, so what do you mean with:
Any refactoring that may make the computation of the return value asynchronous is then a breaking change.
For the state machine tests, the public API is the API of the states. I consider Additionally to that, it is still an abstraction layer even if the module boundary isn't the crate boundary in this case. Breaking changes to module boundaries can still be annoying during refactorings because you suddenly gotta adapt lots of tests or other callsites so I think it is worth it to try and uphold them even if they are internal APIs :) |
Gotcha, that makes sense. Maybe a re-naming is in order? Something that makes it clearer Though |
Terraform Cloud Plan Output
|
Agreed! I was going to send another PR with a docs update to Is just I'll have a word with ChatGPT and see what we can come up with. |
This is a follow-up to #5035. I didn't end up renaming `Tunnel`, `GatewayState` or `ClientState` but I've added some comments with my understanding of what the state is we are tracking and tried to group the fields together in a logical way. --------- Signed-off-by: Thomas Eizinger <thomas@eizinger.io> Co-authored-by: Reactor Scram <ReactorScram@users.noreply.github.com>
The API of connlib is designed around a uni-directional dataflow where commands flow one way and events flow the other way. By design, this creates a system of eventual consistency: We don't exactly know when connlib will emit an event. This is important because it gives us flexibility in what the internals of connlib look like. It also forces the downstream apps to be able to handle any event at any point which avoids bugs where clients rely on a certain order that may just be an implementation detail.
To achieve all of this, it is important that we don't introduce APIs with return values. As soon as a function returns a value, it commits to being able to compute this return value synchronously. Any refactoring that may make the computation of the return value asynchronous is then a breaking change.
Consequently, APIs like
handle_timeout
should never return a value. Instead, they should queue an event that the layer above reacts to accordingly.