Skip to content

Check for background-processor exit condition before+after sleep #2221

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

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

TheBlueMatt
Copy link
Collaborator

In a synchronous BackgroundProcessor, the exit is done by setting an atomic flag, which is most likely to happen while we're asleep. Thus, we previously checked for the exit condition after the sleep (and after we persisted the ChannelManager, if required, though this is no longer required and dates back to when we didn't do a re-persist after breaking out of the main loop).

For an async background-processor, this is also fine, however because of the relatively longer sleep time, if the exit flag is set via a sleep check returning true during event processing, we may end up delaying exit rather substantially.

In order to avoid this, we simply check for the exit condition both before and immediately after the sleep in background-processor.

In a synchronous `BackgroundProcessor`, the exit is done by setting
an atomic flag, which is most likely to happen while we're asleep.
Thus, we previously checked for the exit condition after the sleep
(and after we persisted the `ChannelManager`, if required, though
this is no longer required and dates back to when we didn't do a
re-persist after breaking out of the main loop).

For an async `background-processor`, this is also fine, however
because of the relatively longer sleep time, if the exit flag is
set via a sleep check returning true during event processing, we
may end up delaying exit rather substantially.

In order to avoid this, we simply check for the exit condition both
before and immediately after the sleep in `background-processor`.
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (bc54441) 91.57% compared to head (0553591) 91.58%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2221   +/-   ##
=======================================
  Coverage   91.57%   91.58%           
=======================================
  Files         104      104           
  Lines       51553    51553           
  Branches    51553    51553           
=======================================
+ Hits        47212    47213    +1     
+ Misses       4341     4340    -1     
Impacted Files Coverage Δ
lightning-background-processor/src/lib.rs 83.56% <ø> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TheBlueMatt TheBlueMatt merged commit f1761e0 into lightningdevkit:main Apr 24, 2023
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.

4 participants