-
Notifications
You must be signed in to change notification settings - Fork 477
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 ws killpill #4551
fix ws killpill #4551
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.
❌ Changes requested. Reviewed everything up to ee081c0 in 32 seconds
More details
- Looked at
312
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. backend/windmill-api/src/websocket_triggers.rs:350
- Draft comment:
Be cautious withkillpill_rx.resubscribe()
here. Ifmaybe_listen_to_websocket
is called multiple times, it could lead to multiple subscriptions, which might not be the intended behavior. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new functionlisten_to_unlistened_websockets
which is called both at the start and in the loop ofstart_websockets
. This is a good refactoring for code reuse. However, thekillpill_rx
is resubscribed inmaybe_listen_to_websocket
which might lead to multiple subscriptions if the function is called multiple times. This could be a potential issue if not handled properly.
2. backend/windmill-api/src/websocket_triggers.rs:368
- Draft comment:
Consider whetherbiased;
is necessary here. It prioritizes the first branch, which might not be the intended behavior if equal priority is desired for all branches. - Reason this comment was not posted:
Confidence changes required:50%
Thetokio::select!
macro is used to handle multiple asynchronous operations. However, thebiased;
keyword is used, which prioritizes the first branch. This might not be the intended behavior if equal priority is desired.
3. backend/windmill-api/src/websocket_triggers.rs:537
- Draft comment:
Consider whetherbiased;
is necessary here. It prioritizes the first branch, which might not be the intended behavior if equal priority is desired for all branches. - Reason this comment was not posted:
Confidence changes required:50%
Thetokio::select!
macro is used to handle multiple asynchronous operations. However, thebiased;
keyword is used, which prioritizes the first branch. This might not be the intended behavior if equal priority is desired.
Workflow ID: wflow_FsmCoqI2QcXBJSX0
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 to me! Incremental review on 74f9379 in 9 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. backend/windmill-api/src/websocket_triggers.rs:595
- Draft comment:
Typo fixed in the error message: 'cloesd' corrected to 'closed'. - Reason this comment was not posted:
Confidence changes required:0%
The PR description mentions a fix for a typo in the error message. The typo is indeed present in the original code and has been corrected in the PR.
Workflow ID: wflow_E0UXqWWJTH8Zb3tQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Introduce a kill switch for websocket triggers to enable graceful shutdown and improve trigger management.
killpill_rx
tostart_websockets()
inwebsocket_triggers.rs
for graceful shutdown.listen_to_websocket()
to handlekillpill_rx
for stopping connections.listen_to_unlistened_websockets()
for efficient trigger management.run_server()
inlib.rs
to passws_killpill_rx
tostart_websockets()
.query-5303cb9dd5903aa4791ef8e5e5881a50a832e65c8c9632e2e12cd9c2747f2fc7.json
.This description was created by for 74f9379. It will automatically update as commits are pushed.