Correct `peer_handler::test_process_events_multithreaded` by TheBlueMatt · Pull Request #3254 · lightningdevkit/rust-lightning

TheBlueMatt · 2024-08-19T19:04:49Z

This test was added some time ago in
0c034e9, but never made any sense. PeerManager::process_events will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a process_events call, so having a test that checks that we never go around more than twice is obviously broken.

And, indeed, in CI this tests fails with some regularity.

Instead, the test here is changed to ensure that we detectably go around the loop again at least once.

Fixes#2385

codecov · 2024-08-19T20:11:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (05ed0db) to head (e34519b).
Report is 15 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3254 +/- ##
==========================================
+ Coverage 89.65% 90.23% +0.58% 
==========================================
Files 125 125 Lines 102546 105713 +3167 Branches 102546 105713 +3167 ==========================================
+ Hits 91936 95395 +3459 + Misses 7904 7727 -177 + Partials 2706 2591 -115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2024-08-19T21:10:19Z

- while start_time.elapsed() < Duration::from_millis(100) {
- let val = cfg[0].chan_handler.message_fetch_counter.load(Ordering::Acquire);
- assert!(val <= 2);
- std::thread::yield_now(); // Winblowz seemingly doesn't ever interrupt threads?!


Do we still need this fix?

No, none of the threads completely busy-loop now, they all sleep which should yield++.

This test was added some time ago in 0c034e9, but never made any sense. `PeerManager::process_events` will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a `process_events` call, so having a test that checks that we never go around more than twice is obviously broken. And, indeed, in CI this tests fails with some regularity. Instead, the test here is changed to ensure that we detectably go around the loop again at least once. Fixeslightningdevkit#2385

TheBlueMatt · 2024-08-19T21:14:46Z

Squash-pushed with only the spelling change.

valentinewallace

LGTM. Merging with 1 ack since it's just a test change

TheBlueMatt · 2024-08-19T19:04:49Z

This test was added some time ago in
0c034e9, but never made any sense. PeerManager::process_events will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a process_events call, so having a test that checks that we never go around more than twice is obviously broken.

And, indeed, in CI this tests fails with some regularity.

Instead, the test here is changed to ensure that we detectably go around the loop again at least once.

Fixes#2385

codecov · 2024-08-19T20:11:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (05ed0db) to head (e34519b).
Report is 15 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3254 +/- ##
==========================================
+ Coverage 89.65% 90.23% +0.58% 
==========================================
Files 125 125 Lines 102546 105713 +3167 Branches 102546 105713 +3167 ==========================================
+ Hits 91936 95395 +3459 + Misses 7904 7727 -177 + Partials 2706 2591 -115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2024-08-19T21:10:19Z

- while start_time.elapsed() < Duration::from_millis(100) {
- let val = cfg[0].chan_handler.message_fetch_counter.load(Ordering::Acquire);
- assert!(val <= 2);
- std::thread::yield_now(); // Winblowz seemingly doesn't ever interrupt threads?!


Do we still need this fix?

No, none of the threads completely busy-loop now, they all sleep which should yield++.

This test was added some time ago in 0c034e9, but never made any sense. `PeerManager::process_events` will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a `process_events` call, so having a test that checks that we never go around more than twice is obviously broken. And, indeed, in CI this tests fails with some regularity. Instead, the test here is changed to ensure that we detectably go around the loop again at least once. Fixeslightningdevkit#2385

TheBlueMatt · 2024-08-19T21:14:46Z

Squash-pushed with only the spelling change.

valentinewallace

LGTM. Merging with 1 ack since it's just a test change

TheBlueMatt · 2024-08-19T19:04:49Z

This test was added some time ago in
0c034e9, but never made any sense. PeerManager::process_events will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a process_events call, so having a test that checks that we never go around more than twice is obviously broken.

And, indeed, in CI this tests fails with some regularity.

Instead, the test here is changed to ensure that we detectably go around the loop again at least once.

Fixes#2385

codecov · 2024-08-19T20:11:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (05ed0db) to head (e34519b).
Report is 15 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3254 +/- ##
==========================================
+ Coverage 89.65% 90.23% +0.58% 
==========================================
Files 125 125 Lines 102546 105713 +3167 Branches 102546 105713 +3167 ==========================================
+ Hits 91936 95395 +3459 + Misses 7904 7727 -177 + Partials 2706 2591 -115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2024-08-19T21:10:19Z

- while start_time.elapsed() < Duration::from_millis(100) {
- let val = cfg[0].chan_handler.message_fetch_counter.load(Ordering::Acquire);
- assert!(val <= 2);
- std::thread::yield_now(); // Winblowz seemingly doesn't ever interrupt threads?!


Do we still need this fix?

No, none of the threads completely busy-loop now, they all sleep which should yield++.

This test was added some time ago in 0c034e9, but never made any sense. `PeerManager::process_events` will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a `process_events` call, so having a test that checks that we never go around more than twice is obviously broken. And, indeed, in CI this tests fails with some regularity. Instead, the test here is changed to ensure that we detectably go around the loop again at least once. Fixeslightningdevkit#2385

TheBlueMatt · 2024-08-19T21:14:46Z

Squash-pushed with only the spelling change.

valentinewallace

LGTM. Merging with 1 ack since it's just a test change

TheBlueMatt · 2024-08-19T19:04:49Z

This test was added some time ago in
0c034e9, but never made any sense. PeerManager::process_events will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a process_events call, so having a test that checks that we never go around more than twice is obviously broken.

And, indeed, in CI this tests fails with some regularity.

Instead, the test here is changed to ensure that we detectably go around the loop again at least once.

Fixes#2385

codecov · 2024-08-19T20:11:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (05ed0db) to head (e34519b).
Report is 15 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3254 +/- ##
==========================================
+ Coverage 89.65% 90.23% +0.58% 
==========================================
Files 125 125 Lines 102546 105713 +3167 Branches 102546 105713 +3167 ==========================================
+ Hits 91936 95395 +3459 + Misses 7904 7727 -177 + Partials 2706 2591 -115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2024-08-19T21:10:19Z

- while start_time.elapsed() < Duration::from_millis(100) {
- let val = cfg[0].chan_handler.message_fetch_counter.load(Ordering::Acquire);
- assert!(val <= 2);
- std::thread::yield_now(); // Winblowz seemingly doesn't ever interrupt threads?!


Do we still need this fix?

No, none of the threads completely busy-loop now, they all sleep which should yield++.

This test was added some time ago in 0c034e9, but never made any sense. `PeerManager::process_events` will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a `process_events` call, so having a test that checks that we never go around more than twice is obviously broken. And, indeed, in CI this tests fails with some regularity. Instead, the test here is changed to ensure that we detectably go around the loop again at least once. Fixeslightningdevkit#2385

TheBlueMatt · 2024-08-19T21:14:46Z

Squash-pushed with only the spelling change.

valentinewallace

LGTM. Merging with 1 ack since it's just a test change

TheBlueMatt · 2024-08-19T19:04:49Z

This test was added some time ago in
0c034e9, but never made any sense. PeerManager::process_events will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a process_events call, so having a test that checks that we never go around more than twice is obviously broken.

And, indeed, in CI this tests fails with some regularity.

Instead, the test here is changed to ensure that we detectably go around the loop again at least once.

Fixes#2385

codecov · 2024-08-19T20:11:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (05ed0db) to head (e34519b).
Report is 15 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3254 +/- ##
==========================================
+ Coverage 89.65% 90.23% +0.58% 
==========================================
Files 125 125 Lines 102546 105713 +3167 Branches 102546 105713 +3167 ==========================================
+ Hits 91936 95395 +3459 + Misses 7904 7727 -177 + Partials 2706 2591 -115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2024-08-19T21:10:19Z

- while start_time.elapsed() < Duration::from_millis(100) {
- let val = cfg[0].chan_handler.message_fetch_counter.load(Ordering::Acquire);
- assert!(val <= 2);
- std::thread::yield_now(); // Winblowz seemingly doesn't ever interrupt threads?!


Do we still need this fix?

No, none of the threads completely busy-loop now, they all sleep which should yield++.

This test was added some time ago in 0c034e9, but never made any sense. `PeerManager::process_events` will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a `process_events` call, so having a test that checks that we never go around more than twice is obviously broken. And, indeed, in CI this tests fails with some regularity. Instead, the test here is changed to ensure that we detectably go around the loop again at least once. Fixeslightningdevkit#2385

TheBlueMatt · 2024-08-19T21:14:46Z

Squash-pushed with only the spelling change.

valentinewallace

LGTM. Merging with 1 ack since it's just a test change

TheBlueMatt · 2024-08-19T19:04:49Z

This test was added some time ago in
0c034e9, but never made any sense. PeerManager::process_events will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a process_events call, so having a test that checks that we never go around more than twice is obviously broken.

And, indeed, in CI this tests fails with some regularity.

Instead, the test here is changed to ensure that we detectably go around the loop again at least once.

Fixes#2385

codecov · 2024-08-19T20:11:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (05ed0db) to head (e34519b).
Report is 15 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3254 +/- ##
==========================================
+ Coverage 89.65% 90.23% +0.58% 
==========================================
Files 125 125 Lines 102546 105713 +3167 Branches 102546 105713 +3167 ==========================================
+ Hits 91936 95395 +3459 + Misses 7904 7727 -177 + Partials 2706 2591 -115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2024-08-19T21:10:19Z

- while start_time.elapsed() < Duration::from_millis(100) {
- let val = cfg[0].chan_handler.message_fetch_counter.load(Ordering::Acquire);
- assert!(val <= 2);
- std::thread::yield_now(); // Winblowz seemingly doesn't ever interrupt threads?!


Do we still need this fix?

No, none of the threads completely busy-loop now, they all sleep which should yield++.

This test was added some time ago in 0c034e9, but never made any sense. `PeerManager::process_events` will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a `process_events` call, so having a test that checks that we never go around more than twice is obviously broken. And, indeed, in CI this tests fails with some regularity. Instead, the test here is changed to ensure that we detectably go around the loop again at least once. Fixeslightningdevkit#2385

TheBlueMatt · 2024-08-19T21:14:46Z

Squash-pushed with only the spelling change.

valentinewallace

LGTM. Merging with 1 ack since it's just a test change

TheBlueMatt · 2024-08-19T19:04:49Z

This test was added some time ago in
0c034e9, but never made any sense. PeerManager::process_events will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a process_events call, so having a test that checks that we never go around more than twice is obviously broken.

And, indeed, in CI this tests fails with some regularity.

Instead, the test here is changed to ensure that we detectably go around the loop again at least once.

Fixes#2385

codecov · 2024-08-19T20:11:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (05ed0db) to head (e34519b).
Report is 15 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3254 +/- ##
==========================================
+ Coverage 89.65% 90.23% +0.58% 
==========================================
Files 125 125 Lines 102546 105713 +3167 Branches 102546 105713 +3167 ==========================================
+ Hits 91936 95395 +3459 + Misses 7904 7727 -177 + Partials 2706 2591 -115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2024-08-19T21:10:19Z

- while start_time.elapsed() < Duration::from_millis(100) {
- let val = cfg[0].chan_handler.message_fetch_counter.load(Ordering::Acquire);
- assert!(val <= 2);
- std::thread::yield_now(); // Winblowz seemingly doesn't ever interrupt threads?!


Do we still need this fix?

No, none of the threads completely busy-loop now, they all sleep which should yield++.

This test was added some time ago in 0c034e9, but never made any sense. `PeerManager::process_events` will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a `process_events` call, so having a test that checks that we never go around more than twice is obviously broken. And, indeed, in CI this tests fails with some regularity. Instead, the test here is changed to ensure that we detectably go around the loop again at least once. Fixeslightningdevkit#2385

TheBlueMatt · 2024-08-19T21:14:46Z

Squash-pushed with only the spelling change.

valentinewallace

LGTM. Merging with 1 ack since it's just a test change

TheBlueMatt · 2024-08-19T19:04:49Z

This test was added some time ago in
0c034e9, but never made any sense. PeerManager::process_events will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a process_events call, so having a test that checks that we never go around more than twice is obviously broken.

And, indeed, in CI this tests fails with some regularity.

Instead, the test here is changed to ensure that we detectably go around the loop again at least once.

Fixes#2385

codecov · 2024-08-19T20:11:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (05ed0db) to head (e34519b).
Report is 15 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #3254 +/- ##
==========================================
+ Coverage 89.65% 90.23% +0.58% 
==========================================
Files 125 125 Lines 102546 105713 +3167 Branches 102546 105713 +3167 ==========================================
+ Hits 91936 95395 +3459 + Misses 7904 7727 -177 + Partials 2706 2591 -115

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

valentinewallace · 2024-08-19T21:10:19Z

- while start_time.elapsed() < Duration::from_millis(100) {
- let val = cfg[0].chan_handler.message_fetch_counter.load(Ordering::Acquire);
- assert!(val <= 2);
- std::thread::yield_now(); // Winblowz seemingly doesn't ever interrupt threads?!


Do we still need this fix?

No, none of the threads completely busy-loop now, they all sleep which should yield++.

This test was added some time ago in 0c034e9, but never made any sense. `PeerManager::process_events` will go around its loop as many times is required to ensure we've always processed all events which were pending prior to a `process_events` call, so having a test that checks that we never go around more than twice is obviously broken. And, indeed, in CI this tests fails with some regularity. Instead, the test here is changed to ensure that we detectably go around the loop again at least once. Fixeslightningdevkit#2385

TheBlueMatt · 2024-08-19T21:14:46Z

Squash-pushed with only the spelling change.

valentinewallace

LGTM. Merging with 1 ack since it's just a test change

TheBlueMatt force-pushed the 2024-08-flaky-test branch from 36b4b7e to 9f3ca3dCompare August 19, 2024 21:10

valentinewallace reviewed Aug 19, 2024
View reviewed changes

TheBlueMatt force-pushed the 2024-08-flaky-test branch from 9f3ca3d to e34519bCompare August 19, 2024 21:14

valentinewallace approved these changes Aug 20, 2024
View reviewed changes

valentinewallace merged commit b2cdc2c into lightningdevkit:mainAug 20, 2024