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

cancel all long-running tasks #1793

Merged
merged 5 commits into from Aug 18, 2023
Merged

Conversation

jackstar12
Copy link
Collaborator

@jackstar12 jackstar12 commented Jun 29, 2023

Not all long-running tasks are cancelled upon shutdown, which caused some issues with WALLET.cleanup.

I introduced a few new functions:

  • create_task basically a wrapper around the asyncio equivalent, adds the task to a list
  • cancel_all_tasks cancels all tasks previously created with create_task (if any are already stopped, cancel will do nothing)
  • create_permanent_task wrapper around create_task with catch_everything_and_restart

Now all tasks are cancelled on shutdown and WALLET.cleanup goes without errors.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2023

Codecov Report

Merging #1793 (6ea995c) into main (03acf5e) will decrease coverage by 1.02%.
The diff coverage is 34.48%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #1793      +/-   ##
==========================================
- Coverage   56.09%   55.08%   -1.02%     
==========================================
  Files          48       48              
  Lines        7328     7329       +1     
==========================================
- Hits         4111     4037      -74     
- Misses       3217     3292      +75     
Files Changed Coverage Δ
lnbits/app.py 43.79% <11.11%> (-0.05%) ⬇️
lnbits/tasks.py 33.00% <35.71%> (-0.33%) ⬇️
lnbits/core/tasks.py 24.39% <66.66%> (-1.70%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@dni dni left a comment

Choose a reason for hiding this comment

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

LGTM

lnbits/tasks.py Outdated Show resolved Hide resolved
lnbits/server.py Outdated Show resolved Hide resolved
lnbits/tasks.py Outdated Show resolved Hide resolved
lnbits/wallets/cln.py Outdated Show resolved Hide resolved
jackstar12 and others added 4 commits August 7, 2023 10:37
in order to properly cleanup all long-running tasks we have to keep a list of them
rename variable for create_task
wrap cancel() with try/catch

fixup
@jackstar12
Copy link
Collaborator Author

another interesting side effect of this is that extensions are loaded inside tests aswell, but I dont think this should be considered an issue

@motorina0
Copy link
Collaborator

another interesting side effect of this is that extensions are loaded inside tests aswell, but I dont think this should be considered an issue

  • only extensions listed in LNBITS_EXTENSIONS_DEFAULT_INSTALL should be installed by default
  • if this value is empty no extension will be installed

Copy link
Collaborator

@motorina0 motorina0 left a comment

Choose a reason for hiding this comment

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

LGTM

great job @jackstar12 !

@dni
Copy link
Member

dni commented Aug 7, 2023

another interesting side effect of this is that extensions are loaded inside tests aswell, but I dont think this should be considered an issue

* only extensions listed in `LNBITS_EXTENSIONS_DEFAULT_INSTALL` should be installed by default

* if this value is empty no extension will be installed

good catch, dont install default exts for test when run locally! :)
#1861

@arcbtc arcbtc merged commit 1fd4d9d into lnbits:main Aug 18, 2023
14 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.

None yet

5 participants