Skip to content
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

Add Notifier to OnionMessenger #3194

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Jul 19, 2024

Closes #3190

We add a Notifier to OnionMessenger that allows us to notify the background processor of new events being ready to be handled.

Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 80.85106% with 9 lines in your changes missing coverage. Please review.

Project coverage is 90.95%. Comparing base (9ce3dd5) to head (2dd8c2b).
Report is 80 commits behind head on main.

Files Patch % Lines
lightning-background-processor/src/lib.rs 71.87% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3194      +/-   ##
==========================================
+ Coverage   89.78%   90.95%   +1.17%     
==========================================
  Files         121      122       +1     
  Lines      100932   109280    +8348     
  Branches   100932   109280    +8348     
==========================================
+ Hits        90619    99395    +8776     
+ Misses       7635     7191     -444     
- Partials     2678     2694      +16     

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

@tnull tnull force-pushed the 2024-07-om-event-notifier branch 2 times, most recently from 7a7a238 to 134b46b Compare July 19, 2024 12:46
@TheBlueMatt
Copy link
Collaborator

Need to update the non-async BP too.

@tnull
Copy link
Contributor Author

tnull commented Jul 19, 2024

Need to update the non-async BP too.

Wat, we still support that? :P

(Ah, thanks, good catch!)

@tnull tnull force-pushed the 2024-07-om-event-notifier branch from dcadadb to df38fd5 Compare July 19, 2024 13:31
arik-so
arik-so previously approved these changes Jul 30, 2024
@tnull
Copy link
Contributor Author

tnull commented Aug 5, 2024

Let me know if I can squash fixups

@TheBlueMatt
Copy link
Collaborator

Feel free, IMO.

@TheBlueMatt TheBlueMatt added this to the 0.0.124 milestone Aug 5, 2024
@tnull tnull force-pushed the 2024-07-om-event-notifier branch 3 times, most recently from d934b96 to b3d45cc Compare August 5, 2024 17:30
@tnull
Copy link
Contributor Author

tnull commented Aug 5, 2024

Squashed without further changes (ignore the intermittent force-pushes..):

> git diff-tree -U2 e3ed93b2d b3d45ccfd
>

@tnull tnull force-pushed the 2024-07-om-event-notifier branch from b3d45cc to 7b8200a Compare August 7, 2024 07:07
@tnull
Copy link
Contributor Author

tnull commented Aug 7, 2024

Force-pushed with the following changes:

> git diff-tree -U2 b3d45ccfd 7b8200a47
diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs
index 30ad572fd..ec1682bed 100644
--- a/lightning/src/onion_message/messenger.rs
+++ b/lightning/src/onion_message/messenger.rs
@@ -1250,8 +1250,5 @@ where
                        },
                        hash_map::Entry::Occupied(mut e) => {
-                               let notify = e.get_mut().enqueue_message(onion_message);
-                               if notify {
-                                       self.event_notifier.notify();
-                               }
+                               e.get_mut().enqueue_message(onion_message);
                                if e.get().is_connected() {
                                        Ok(SendSuccess::Buffered)

@tnull tnull force-pushed the 2024-07-om-event-notifier branch from 7b8200a to 2dd8c2b Compare August 8, 2024 07:10
@tnull
Copy link
Contributor Author

tnull commented Aug 8, 2024

Force-pushed with additional changes (actually reverting prior changes):

> git diff-tree -U2 7b8200a47 2dd8c2b3d
diff --git a/lightning/src/onion_message/messenger.rs b/lightning/src/onion_message/messenger.rs
index ec1682bed..590886721 100644
--- a/lightning/src/onion_message/messenger.rs
+++ b/lightning/src/onion_message/messenger.rs
@@ -295,17 +295,11 @@ impl OnionMessageRecipient {
        }

-       // Returns whether changes were made that are pending event processing
-       fn enqueue_message(&mut self, message: OnionMessage) -> bool {
-               let mut pending_event_processing = false;
+       fn enqueue_message(&mut self, message: OnionMessage) {
                let pending_messages = match self {
                        OnionMessageRecipient::ConnectedPeer(pending_messages) => pending_messages,
-                       OnionMessageRecipient::PendingConnection(pending_messages, _, _) => {
-                               pending_event_processing = true;
-                               pending_messages
-                       }
+                       OnionMessageRecipient::PendingConnection(pending_messages, _, _) => pending_messages,
                };

                pending_messages.push_back(message);
-               pending_event_processing
        }

@@ -1241,9 +1235,7 @@ where
                                None => Err(SendError::InvalidFirstHop(first_node_id)),
                                Some(addresses) => {
-                                       let notify = e.insert(OnionMessageRecipient::pending_connection(addresses))
+                                       e.insert(OnionMessageRecipient::pending_connection(addresses))
                                                .enqueue_message(onion_message);
-                                       if notify {
-                                               self.event_notifier.notify();
-                                       }
+                                       self.event_notifier.notify();
                                        Ok(SendSuccess::BufferedAwaitingConnection(first_node_id))
                                },

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TheBlueMatt TheBlueMatt merged commit 1d9d911 into lightningdevkit:main Aug 8, 2024
14 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OnionMessenger needs a Notifier
3 participants