-
Notifications
You must be signed in to change notification settings - Fork 141
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 sigterm issue with translator sv2 #1319
fix sigterm issue with translator sv2 #1319
Conversation
Bencher Report
🚨 1 Alert
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1319 +/- ##
==========================================
- Coverage 19.11% 3.79% -15.33%
==========================================
Files 166 44 -122
Lines 11047 2582 -8465
==========================================
- Hits 2112 98 -2014
+ Misses 8935 2484 -6451
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Please make this PR easier to review by undoing the formatting/indentation changes. If you are moving a function outside the TranslatorSV2
struct, do it in a separate commit and then commit the changes to make the fix
3ef1198
to
2a0d255
Compare
Bencher Report
Click to view all benchmark results
|
2a0d255
to
04455f0
Compare
tACK |
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. Just one small nit.
IMO before we merge this or #1321, a test to cover the feature/bug fix should be implemented.
04455f0
to
282aa52
Compare
that is a good point, overall I'm always supportive of the idea of always reproducing bugs and enforcing sane behavior on edge cases via CI but is that something that could be achieved via integration tests? I can imagine the following flow:
however I'm not sure how to achieve this inside the test environment via Rust code... is there a way to programatically emulate a sigterm that would only be directed to tProxy and not the entire test execution? (btw I'm not arguing it's not possible, just haven't done the research no I don't have full confidence on my reasoning) |
This can be done. I have tried sending terminal signals to the process, but I’m unsure about including that in this PR or adding any testing for it. The primary scope of this PR was to resolve the |
This PR is resolving(partially, the translator part) #1266. Just a reminder, for #1207 I was able to write the test fully and run it successfully, but the |
@Shourya742 #1343 this branch have your code and the test(for #1207) I wrote back then(I did some changes to align it with current API design). Feel free to pick it up, otherwise Ill look into it tomorrow. |
Let me complete it then, I know what might be the pain point. If my analysis is correct. Will let you know. Thanks for this.. |
66dada7
to
ab1d4b8
Compare
ab1d4b8
to
8c7b53b
Compare
8c7b53b
to
80b67d4
Compare
Partially closes: #1266
The issue lies with the blocking nature of
self.internal_start().await
inside the loop. Since the .await suspends the current task, it prevents the tokio::select! from responding promptly to other events, such as the ctrl_c() signal. This PR tends to spawn to internal_start as a separate task, while still keeping the select! macro active to listen for other events.