fix jdc sigterm signalling issue #1321
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Bencher Report
|
| Branch | 2024-12-23-fix-sigterm-jdc |
| Testbed | sv2 |
🚨 1 Alert
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| client_sv2_handle_message_mining | L2 Accesses accesses | 📈 plot 🚨 alert (🔔) 🚷 threshold | 43.00 (+20.57%) | 41.90 (102.61%) |
Click to view all benchmark results
| Benchmark | Estimated Cycles | Benchmark Result 1e3 x estimated cycles (Result Δ%) | Upper Boundary 1e3 x estimated cycles (Limit %) | Instructions | Benchmark Result instructions (Result Δ%) | Upper Boundary instructions (Limit %) | L1 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | L2 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | RAM Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| client_sv2_handle_message_common | 📈 view plot 🚷 view threshold | 2.17 (+2.61%) | 2.24 (96.91%) | 📈 view plot 🚷 view threshold | 473.00 (-0.14%) | 490.49 (96.43%) | 📈 view plot 🚷 view threshold | 731.00 (-0.68%) | 758.92 (96.32%) | 📈 view plot 🚷 view threshold | 8.00 (+55.15%) | 11.67 (68.57%) | 📈 view plot 🚷 view threshold | 40.00 (+3.39%) | 41.79 (95.72%) |
| client_sv2_handle_message_mining | 📈 view plot 🚷 view threshold | 8.33 (+1.29%) | 8.40 (99.17%) | 📈 view plot 🚷 view threshold | 2,137.00 | 📈 view plot 🚷 view threshold | 3,149.00 (-0.30%) | 3,167.64 (99.41%) | 📈 view plot 🚨 view alert (🔔) 🚷 view threshold | 43.00 (+20.57%) | 41.90 (102.61%) | 📈 view plot 🚷 view threshold | 142.00 (+1.62%) | 144.63 (98.18%) | |
| client_sv2_mining_message_submit_standard | 📈 view plot 🚷 view threshold | 6.37 (+1.04%) | 6.45 (98.73%) | 📈 view plot 🚷 view threshold | 1,750.00 (-0.04%) | 1,767.49 (99.01%) | 📈 view plot 🚷 view threshold | 2,544.00 (-0.31%) | 2,575.38 (98.78%) | 📈 view plot 🚷 view threshold | 23.00 (+33.70%) | 24.24 (94.88%) | 📈 view plot 🚷 view threshold | 106.00 (+1.22%) | 108.82 (97.41%) |
| client_sv2_mining_message_submit_standard_serialize | 📈 view plot 🚷 view threshold | 14.83 (+0.69%) | 14.94 (99.26%) | 📈 view plot 🚷 view threshold | 4,694.00 (-0.01%) | 4,711.49 (99.63%) | 📈 view plot 🚷 view threshold | 6,743.00 (-0.19%) | 6,786.79 (99.35%) | 📈 view plot 🚷 view threshold | 56.00 (+21.86%) | 63.59 (88.06%) | 📈 view plot 🚷 view threshold | 223.00 (+0.82%) | 226.16 (98.60%) |
| client_sv2_mining_message_submit_standard_serialize_deserialize | 📈 view plot 🚷 view threshold | 27.79 (+0.65%) | 28.07 (99.00%) | 📈 view plot 🚷 view threshold | 10,645.00 (+0.35%) | 10,700.14 (99.48%) | 📈 view plot 🚷 view threshold | 15,503.00 (+0.39%) | 15,596.66 (99.40%) | 📈 view plot 🚷 view threshold | 91.00 (+8.43%) | 99.35 (91.59%) | 📈 view plot 🚷 view threshold | 338.00 (+0.72%) | 343.27 (98.47%) |
| client_sv2_open_channel | 📈 view plot 🚷 view threshold | 4.46 (+1.14%) | 4.59 (97.12%) | 📈 view plot 🚷 view threshold | 1,461.00 (-0.05%) | 1,478.49 (98.82%) | 📈 view plot 🚷 view threshold | 2,155.00 (-0.26%) | 2,184.67 (98.64%) | 📈 view plot 🚷 view threshold | 12.00 (+45.45%) | 14.09 (85.18%) | 📈 view plot 🚷 view threshold | 64.00 (+1.68%) | 67.89 (94.27%) |
| client_sv2_open_channel_serialize | 📈 view plot 🚷 view threshold | 14.08 (+0.41%) | 14.20 (99.12%) | 📈 view plot 🚷 view threshold | 5,064.00 (-0.01%) | 5,081.49 (99.66%) | 📈 view plot 🚷 view threshold | 7,318.00 (-0.11%) | 7,352.75 (99.53%) | 📈 view plot 🚷 view threshold | 43.00 (+16.88%) | 48.49 (88.68%) | 📈 view plot 🚷 view threshold | 187.00 (+0.53%) | 190.88 (97.97%) |
| client_sv2_open_channel_serialize_deserialize | 📈 view plot 🚷 view threshold | 22.82 (+0.49%) | 23.08 (98.91%) | 📈 view plot 🚷 view threshold | 8,040.00 (+0.09%) | 8,057.61 (99.78%) | 📈 view plot 🚷 view threshold | 11,689.00 (+0.03%) | 11,713.53 (99.79%) | 📈 view plot 🚷 view threshold | 85.00 (+11.92%) | 90.08 (94.36%) | 📈 view plot 🚷 view threshold | 306.00 (+0.59%) | 312.90 (97.79%) |
| client_sv2_setup_connection | 📈 view plot 🚷 view threshold | 4.72 (+0.56%) | 4.79 (98.45%) | 📈 view plot 🚷 view threshold | 1,502.00 (-0.04%) | 1,519.49 (98.85%) | 📈 view plot 🚷 view threshold | 2,274.00 (-0.21%) | 2,300.86 (98.83%) | 📈 view plot 🚷 view threshold | 13.00 (+37.29%) | 15.35 (84.72%) | 📈 view plot 🚷 view threshold | 68.00 (+0.57%) | 70.18 (96.90%) |
| client_sv2_setup_connection_serialize | 📈 view plot 🚷 view threshold | 16.26 (+0.61%) | 16.33 (99.53%) | 📈 view plot 🚷 view threshold | 5,963.00 (-0.01%) | 5,980.49 (99.71%) | 📈 view plot 🚷 view threshold | 8,651.00 (-0.15%) | 8,692.74 (99.52%) | 📈 view plot 🚷 view threshold | 51.00 (+24.82%) | 56.60 (90.10%) | 📈 view plot 🚷 view threshold | 210.00 (+0.82%) | 212.24 (98.95%) |
| client_sv2_setup_connection_serialize_deserialize | 📈 view plot 🚷 view threshold | 35.75 (+0.40%) | 35.96 (99.41%) | 📈 view plot 🚷 view threshold | 14,888.00 (+0.13%) | 14,920.01 (99.79%) | 📈 view plot 🚷 view threshold | 21,868.00 (+0.11%) | 21,919.56 (99.76%) | 📈 view plot 🚷 view threshold | 109.00 (+14.95%) | 120.30 (90.60%) | 📈 view plot 🚷 view threshold | 381.00 (+0.35%) | 385.77 (98.76%) |
|
| Branch | 2024-12-23-fix-sigterm-jdc |
| Testbed | sv1 |
Click to view all benchmark results
| Benchmark | Estimated Cycles | Benchmark Result 1e3 x estimated cycles (Result Δ%) | Upper Boundary 1e3 x estimated cycles (Limit %) | Instructions | Benchmark Result 1e3 x instructions (Result Δ%) | Upper Boundary 1e3 x instructions (Limit %) | L1 Accesses | Benchmark Result 1e3 x accesses (Result Δ%) | Upper Boundary 1e3 x accesses (Limit %) | L2 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | RAM Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| get_authorize | 📈 view plot 🚷 view threshold | 8.36 (-1.10%) | 8.67 (96.41%) | 📈 view plot 🚷 view threshold | 3.69 (-0.92%) | 3.86 (95.54%) | 📈 view plot 🚷 view threshold | 5.16 (-1.06%) | 5.45 (94.67%) | 📈 view plot 🚷 view threshold | 11.00 (+28.70%) | 16.47 (66.77%) | 📈 view plot 🚷 view threshold | 90.00 (-1.56%) | 96.80 (92.97%) |
| get_submit | 📈 view plot 🚷 view threshold | 95.16 (-0.21%) | 95.62 (99.52%) | 📈 view plot 🚷 view threshold | 59.35 (-0.09%) | 59.71 (99.39%) | 📈 view plot 🚷 view threshold | 85.22 (-0.09%) | 85.82 (99.31%) | 📈 view plot 🚷 view threshold | 47.00 (+4.64%) | 60.46 (77.73%) | 📈 view plot 🚷 view threshold | 277.00 (-1.33%) | 291.89 (94.90%) |
| get_subscribe | 📈 view plot 🚷 view threshold | 7.85 (-1.82%) | 8.23 (95.32%) | 📈 view plot 🚷 view threshold | 2.77 (-1.63%) | 2.94 (93.98%) | 📈 view plot 🚷 view threshold | 3.85 (-1.90%) | 4.14 (92.86%) | 📈 view plot 🚷 view threshold | 16.00 (+26.73%) | 20.95 (76.39%) | 📈 view plot 🚷 view threshold | 112.00 (-2.20%) | 117.98 (94.93%) |
| serialize_authorize | 📈 view plot 🚷 view threshold | 12.09 (-1.39%) | 12.51 (96.63%) | 📈 view plot 🚷 view threshold | 5.27 (-0.58%) | 5.43 (97.05%) | 📈 view plot 🚷 view threshold | 7.33 (-0.68%) | 7.60 (96.37%) | 📈 view plot 🚷 view threshold | 14.00 (+35.55%) | 19.02 (73.62%) | 📈 view plot 🚷 view threshold | 134.00 (-2.85%) | 143.40 (93.45%) |
| serialize_deserialize_authorize | 📈 view plot 🚷 view threshold | 24.60 (-0.45%) | 25.19 (97.68%) | 📈 view plot 🚷 view threshold | 9.84 (-0.18%) | 10.01 (98.28%) | 📈 view plot 🚷 view threshold | 13.88 (-0.21%) | 14.17 (97.90%) | 📈 view plot 🚷 view threshold | 38.00 (+5.60%) | 45.99 (82.62%) | 📈 view plot 🚷 view threshold | 301.00 (-0.86%) | 313.67 (95.96%) |
| serialize_deserialize_handle_authorize | 📈 view plot 🚷 view threshold | 30.08 (-0.80%) | 30.73 (97.87%) | 📈 view plot 🚷 view threshold | 12.02 (-0.28%) | 12.19 (98.60%) | 📈 view plot 🚷 view threshold | 17.00 (-0.32%) | 17.30 (98.30%) | 📈 view plot 🚷 view threshold | 60.00 (+7.37%) | 67.53 (88.85%) | 📈 view plot 🚷 view threshold | 365.00 (-1.61%) | 379.60 (96.15%) |
| serialize_deserialize_handle_submit | 📈 view plot 🚷 view threshold | 126.28 (-0.14%) | 126.79 (99.60%) | 📈 view plot 🚷 view threshold | 73.20 (-0.06%) | 73.53 (99.55%) | 📈 view plot 🚷 view threshold | 104.91 (-0.08%) | 105.51 (99.43%) | 📈 view plot 🚷 view threshold | 116.00 (+8.51%) | 126.06 (92.02%) | 📈 view plot 🚷 view threshold | 594.00 (-0.63%) | 610.68 (97.27%) |
| serialize_deserialize_handle_subscribe | 📈 view plot 🚷 view threshold | 27.71 (-0.72%) | 28.39 (97.61%) | 📈 view plot 🚷 view threshold | 9.58 (-0.47%) | 9.76 (98.21%) | 📈 view plot 🚷 view threshold | 13.54 (-0.55%) | 13.84 (97.83%) | 📈 view plot 🚷 view threshold | 69.00 (+6.74%) | 78.13 (88.31%) | 📈 view plot 🚷 view threshold | 395.00 (-1.05%) | 409.82 (96.38%) |
| serialize_deserialize_submit | 📈 view plot 🚷 view threshold | 115.14 (-0.09%) | 115.71 (99.51%) | 📈 view plot 🚷 view threshold | 68.06 (+0.01%) | 68.42 (99.47%) | 📈 view plot 🚷 view threshold | 97.65 (+0.00%) | 98.29 (99.35%) | 📈 view plot 🚷 view threshold | 69.00 (+4.93%) | 87.84 (78.56%) | 📈 view plot 🚷 view threshold | 490.00 (-0.70%) | 505.36 (96.96%) |
| serialize_deserialize_subscribe | 📈 view plot 🚷 view threshold | 23.15 (-0.75%) | 23.82 (97.18%) | 📈 view plot 🚷 view threshold | 8.14 (-0.50%) | 8.32 (97.93%) | 📈 view plot 🚷 view threshold | 11.45 (-0.59%) | 11.75 (97.49%) | 📈 view plot 🚷 view threshold | 43.00 (+9.90%) | 51.32 (83.79%) | 📈 view plot 🚷 view threshold | 328.00 (-1.10%) | 342.48 (95.77%) |
| serialize_submit | 📈 view plot 🚷 view threshold | 99.54 (-0.25%) | 100.10 (99.44%) | 📈 view plot 🚷 view threshold | 61.41 (-0.07%) | 61.73 (99.47%) | 📈 view plot 🚷 view threshold | 88.08 (-0.08%) | 88.65 (99.37%) | 📈 view plot 🚷 view threshold | 51.00 (+5.56%) | 67.16 (75.93%) | 📈 view plot 🚷 view threshold | 320.00 (-1.66%) | 337.67 (94.77%) |
| serialize_subscribe | 📈 view plot 🚷 view threshold | 11.29 (-0.97%) | 11.60 (97.31%) | 📈 view plot 🚷 view threshold | 4.12 (-1.01%) | 4.28 (96.16%) | 📈 view plot 🚷 view threshold | 5.71 (-1.23%) | 6.00 (95.17%) | 📈 view plot 🚷 view threshold | 17.00 (+20.96%) | 24.45 (69.53%) | 📈 view plot 🚷 view threshold | 157.00 (-0.98%) | 164.64 (95.36%) |
|
| Branch | 2024-12-23-fix-sigterm-jdc |
| Testbed | sv2 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| client_sv2_handle_message_common | 📈 view plot 🚷 view threshold | 44.10 (-2.95%) | 62.09 (71.02%) |
| client_sv2_handle_message_mining | 📈 view plot 🚷 view threshold | 75.28 (-0.33%) | 98.47 (76.45%) |
| client_sv2_mining_message_submit_standard | 📈 view plot 🚷 view threshold | 14.63 (-0.22%) | 14.73 (99.34%) |
| client_sv2_mining_message_submit_standard_serialize | 📈 view plot 🚷 view threshold | 270.15 (+2.54%) | 287.21 (94.06%) |
| client_sv2_mining_message_submit_standard_serialize_deserialize | 📈 view plot 🚷 view threshold | 632.73 (+1.85%) | 686.25 (92.20%) |
| client_sv2_open_channel | 📈 view plot 🚷 view threshold | 166.49 (+0.37%) | 179.87 (92.56%) |
| client_sv2_open_channel_serialize | 📈 view plot 🚷 view threshold | 282.29 (-1.52%) | 318.78 (88.55%) |
| client_sv2_open_channel_serialize_deserialize | 📈 view plot 🚷 view threshold | 385.50 (+0.66%) | 399.40 (96.52%) |
| client_sv2_setup_connection | 📈 view plot 🚷 view threshold | 156.77 (-2.03%) | 171.55 (91.39%) |
| client_sv2_setup_connection_serialize | 📈 view plot 🚷 view threshold | 493.95 (+5.14%) | 556.66 (88.74%) |
| client_sv2_setup_connection_serialize_deserialize | 📈 view plot 🚷 view threshold | 977.27 (-3.52%) | 1,249.32 (78.22%) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1321 +/- ##
==========================================
+ Coverage 16.90% 19.11% +2.21%
==========================================
Files 164 166 +2
Lines 10956 11047 +91
==========================================
+ Hits 1852 2112 +260
+ Misses 9104 8935 -169
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Shourya742
force-pushed
the
2024-12-23-fix-sigterm-jdc
branch
from
3aa0262 to
6bb7eb0
Compare
jbesraa
suggested changes
Dec 30, 2024
Shourya742
force-pushed
the
2024-12-23-fix-sigterm-jdc
branch
from
6bb7eb0 to
65512c1
Compare
plebhash
reviewed
Jan 3, 2025
plebhash
reviewed
Jan 3, 2025
plebhash
reviewed
Jan 3, 2025
Shourya742
force-pushed
the
2024-12-23-fix-sigterm-jdc
branch
from
65512c1 to
a3efa46
Compare
plebhash
reviewed
Jan 6, 2025
|
tACK I found a typo, but overall LGTM |
plebhash
approved these changes
Jan 6, 2025
jbesraa
suggested changes
Jan 9, 2025
There was a problem hiding this comment.
Looking good overall. Added a few nits.
Regarding this commit 6a95deb could you please explain why it is added? the title says "move task_status block inside the select macro" but it is not clear why the task_status was moved. maybe add that in the commit body.
a0ffe75 could have a better title, maybe "Fix typo", or you could just fix it up to the second commit(a3efa46).
Shourya742
force-pushed
the
2024-12-23-fix-sigterm-jdc
branch
from
a0ffe75 to
0352e78
Compare
|
@jbesraa I have incorporated your suggestions and updated the commit history to a single commit since the changes are closely related. |
Shourya742
force-pushed
the
2024-12-23-fix-sigterm-jdc
branch
from
0352e78 to
a9fb6f4
Compare
jbesraa
suggested changes
Jan 13, 2025
Shourya742
force-pushed
the
2024-12-23-fix-sigterm-jdc
branch
from
0e02c9f to
b1e6780
Compare
jbesraa
approved these changes
Jan 13, 2025
plebhash
approved these changes
Jan 14, 2025
It changes the structuring of the start method, to avoid its blocking nature in case of disconnection from the upstream. Currently, when we are sending the termination signal, during the stance of disconnection from upstream, due of blocking nature of initialize_jd it halts the main thread runtime from executing select block, which listens for any termination signal and channel responses. -Modifications 1. Addition of new shutdown field, which can be used later in integration test to terminate the instances 2. Change argument type for methods initialize_jd and initialize_jd_solo, to make them movable. 3. Spawning of blocking process as a separate task
pavlenex
force-pushed
the
2024-12-23-fix-sigterm-jdc
branch
from
b1e6780 to
6d1632b
Compare
jbesraa
reviewed
Jan 16, 2025
| }, | ||
| _ = self.shutdown.notified().fuse() => { | ||
| info!("Shutting down gracefully..."); | ||
| std::process::exit(0); |
There was a problem hiding this comment.
This should not panic, otherwise it will make tests exist fully. Maybe change to break;
There was a problem hiding this comment.
I had this in mind while making the change. I might have mentioned before that this could lead to a complete process exit. Ideally, we should consider breaking out of the outer loop instead.
There was a problem hiding this comment.
yea. break should be enough IMO as we are already in the outer loop
There was a problem hiding this comment.
Cooll... I will raise one asap
| @@ -347,6 +367,10 @@ impl JobDeclaratorClient { | |||
| ) | |||
| .await; | |||
| } | |||
|
|
|||
| pub fn shutdown(&self) -> Arc<Notify> { | |||
There was a problem hiding this comment.
After trying this locally, I think two changes need to be done:
- this funciton should not return anything. it should perform the shutdown.
- a unit test to cover this function should be added. If this is working as expected, this unit test should pass:
#[cfg(test)]
mod tests {
use ext_config::{Config, File, FileFormat};
use crate::*;
#[test]
fn test_shutdown() {
let config_path = "config-examples/jdc-config-hosted-example.toml";
let config: ProxyConfig = match Config::builder()
.add_source(File::new(config_path, FileFormat::Toml))
.build()
{
Ok(settings) => match settings.try_deserialize::<ProxyConfig>() {
Ok(c) => c,
Err(e) => {
dbg!(&e);
return;
}
},
Err(e) => {
dbg!(&e);
return;
}
};
let jdc = JobDeclaratorClient::new(config.clone());
let cloned = jdc.clone();
tokio::spawn(async move {
cloned.start().await;
});
jdc.shutdown();
let ip = config.downstream_address.clone();
let port = config.downstream_port;
let jdc_addr = format!("{}:{}", ip, port);
assert!(std::net::TcpListener::bind(jdc_addr).is_ok());
}
}There was a problem hiding this comment.
I added a return type to provide a shutdown handle to the user or, in the case of integration tests, to enable more efficient use of signaling in the future—not just for shutdown scenarios. With this kind of external signaling, we can achieve much more. Therefore, I felt it made sense to pass the handle to the end-user. Let me know your thoughts on this!
There was a problem hiding this comment.
What other scenarios could arise? if they exist then a new function should be added.
There was a problem hiding this comment.
What other scenarios could arise? if they exist then a new function should be added.
Not entirely sure now... But if we ever want to do some channel control...we can achieve that with this signalling mechanisms that we now have in place...
There was a problem hiding this comment.
the shutdown signal is only notified here https://github.com/stratum-mining/stratum/pull/1321/files/6d1632b2eaca205f4b0e964a7a8d1d4c0b2c5d3a#diff-b344c0ca590735a830cbfb638690b53af5889deeef68717d4586b5da8af62479R171, which is very correct. but it also mean that anyone who is using the shutdown object, can only perform shutdown action. which is also correct, we don't want the same signal to perform different actions.
This was referenced Jan 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes: #1266
Description
This PR introduces several key changes to the
JobDeclaratorClientimplementation to enhance its functionality, improve shutdown handling, and resolve blocking issues during task execution:Use of
tokio::spawnfor Task Initializationinitialize_jdandinitialize_jd_as_solo_minermethods are now executed usingtokio::spawn.Refactored Method Signatures
initialize_jdandinitialize_jd_as_solo_minerto acceptProxyConfigas a parameter instead of relying on&self.selffrom being moved into the spawned tasks and allows for proper cloning and reuse within the async context.Graceful Shutdown Mechanism
tokio::sync::Notifyinstance as part ofJobDeclaratorClientstruct to handle shutdown signals. Which now gives power to struct initializer to shutdown process directly.ctrl_csignal handling with a task that listens for the signal and triggers the shutdown notifier.Simplified Interrupt Signal Handling
ctrl_cinterrupt futures within the select loop.