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

feat: example app and waku_destroy #99

Merged
merged 1 commit into from
Mar 11, 2024
Merged

feat: example app and waku_destroy #99

merged 1 commit into from
Mar 11, 2024

Conversation

richard-ramos
Copy link
Member

No description provided.

Copy link

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

Very nice thanks!

Would it be useful to store the node config in WakuNodehandle, only on Running nodes allow call to waku_destroy then return an Initialized node?

Copy link

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Very educational PR, thanks for it!
qq: why waku_destroy is needed? We don't have such concept in nwaku-libwaku.

@@ -43,6 +43,18 @@ pub fn waku_new(config: Option<WakuNodeConfig>) -> Result<WakuNodeContext> {
}
}

pub fn waku_destroy(ctx: &WakuNodeContext) -> Result<()> {

Choose a reason for hiding this comment

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

Maybe it worth adding a little comment above

@richard-ramos
Copy link
Member Author

qq: why waku_destroy is needed? We don't have such concept in nwaku-libwaku.

The reason why it is necessary is because in waku_new we are creating a context running its own thread, yet we're not stopping that thread nor releasing the resources allocated by the Context by calling stopWakuNodeThread .

In waku-org/nwaku#2499 I added the waku_destroy function which calls that function, so we can do a proper cleanup of the resources used by waku when we no longer require a node. (Think of it as a graceful shutdown)

@Ivansete-status
Copy link

qq: why waku_destroy is needed? We don't have such concept in nwaku-libwaku.

The reason why it is necessary is because in waku_new we are creating a context running its own thread, yet we're not stopping that thread nor releasing the resources allocated by the Context by calling stopWakuNodeThread .

In waku-org/nwaku#2499 I added the waku_destroy function which calls that function, so we can do a proper cleanup of the resources used by waku when we no longer require a node. (Think of it as a graceful shutdown)

Wonderful! Thanks for that!

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 15.07937% with 107 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (nwaku@69a4872). Click here to learn what that means.

Files Patch % Lines
examples/basic/src/main.rs 0.00% 107 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             nwaku      #99   +/-   ##
========================================
  Coverage         ?   70.00%           
========================================
  Files            ?       11           
  Lines            ?      860           
  Branches         ?        0           
========================================
  Hits             ?      602           
  Misses           ?      258           
  Partials         ?        0           

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

@richard-ramos richard-ramos merged commit 201a38a into nwaku Mar 11, 2024
4 of 8 checks passed
@richard-ramos richard-ramos deleted the example-app branch March 11, 2024 16:00
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