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

refactor(cbindings): libwaku - run waku node in a secondary working thread #1865

Merged
merged 4 commits into from
Jul 31, 2023

Conversation

Ivansete-status
Copy link
Collaborator

Description

This refactoring is needed as a previous step to allow the integration of libwaku into nodejs.

@zah gave me a great hint regarding the interoperability between NodeJs and Nim:

It's certainly not possible to run both node.js code and Nim async code in the same thread, because both run-times want to take control of the thread for the purposes of calling epoll

Changes

  • Create Waku Node thread
  • Connect the libwaku user-space-main-thread with the Waku Node thread throughout channels.
  • The libwaku user won't need to call waku_init nor waku_poll.

Issue

#1632

@Ivansete-status Ivansete-status marked this pull request as ready for review July 28, 2023 13:56
@Ivansete-status
Copy link
Collaborator Author

I still need to validate this works correctly in NodeJs. I tested it in another feature branch and it could receive messages during ~1h but I will make a longer test (1 day.)
At least this approach is more promising than using the main thread. In this scenario, the NodeJs example crashed in 30''.

Copy link
Contributor

@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.

LGTM but that doesn't mean much. I've only dealt with c bindings once before in Rust.

I like the channel approach.

Copy link
Contributor

@rymnc rymnc left a comment

Choose a reason for hiding this comment

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

LGTM, but don't have much expertise in this area!

@Ivansete-status Ivansete-status merged commit 069c1ad into master Jul 31, 2023
9 checks passed
@Ivansete-status Ivansete-status deleted the feat-libwaku-in-separate-thread branch July 31, 2023 07:52
respChannel: Channel[Result[string, string]]
node: WakuNode

var ctx {.threadvar.}: ptr Context
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using a global here, it's better to return the context to the C api and ask them to pass it in to every function

type
Context* = object
thread: Thread[(ptr Context)]
reqChannel: Channel[InterThreadRequest]
Copy link
Contributor

Choose a reason for hiding this comment

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

Channel uses some memory allocator tricks that are not necessarily safe - it has been removed in v2 / moved outside of the standard library because it's design has some problems - in general, it's likely better to use a serialization library that creates a flat representation of the data, allocates a shared memory section and passes that to the other thread - the easiest one to start with is probably json-serialization - later a more efficient one can be selected.

https://gist.github.com/arnetheduck/b6a7ac8f4b85490d26d464674e09d57d is a simple example of this approach.

In general, ref types are allocated on a thread-local heap and using them in cross-thread scenarios is not well-tested in nim - it's safer to stick with non-ref types completely and rely on "manual" allocation with allocShared / createShared for this purpose.

We will eventually develop a common high-performance channel, but that doesn't really matter for a use case like waku.

running.store(true)

try:
createThread(ctx.thread, run, ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

the context contains garbage-collected types - therefore, it is not safe to pass it to a different thread where references to these instances may accidentally be copied.

In particualr, the node itself must not be allocated in this thread and then used in the other thread - this will lead to memory corruption.

Instead, the Node instance should be created inside run, in the thread where it's running - to configure it, a configuration should be passed to it that is allocated with allocShared and doesn't contain any refcounted types.

The rule is: never allocate anything with new (no ref object, etc) in one thread and use it in the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

To elaborate, Context should only contain thread-safe types, ie those that are not string, seq, and ref. It can contain ptr, array and other "manually" allocated types.

../waku_thread

type
InterThreadRequest* = ref object of RootObj
Copy link
Contributor

Choose a reason for hiding this comment

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

as noted elsewhere, this should not be a ref object - rather, the easiest thing to use is a json blob that gets allocated with allocShared or the helper you created for it (alloc)

@arnetheduck
Copy link
Contributor

Nice! We'll need some adjustments to this approach to make it thread-safe and not break garbage collection in unexpected ways, but we're on the right path here.

@Ivansete-status
Copy link
Collaborator Author

Nice! We'll need some adjustments to this approach to make it thread-safe and not break garbage collection in unexpected ways, but we're on the right path here.

Thanks for the comments! We'll apply them asap!

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

4 participants