-
Notifications
You must be signed in to change notification settings - Fork 54
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
Conversation
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.) |
There was a problem hiding this 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.
There was a problem hiding this 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!
respChannel: Channel[Result[string, string]] | ||
node: WakuNode | ||
|
||
var ctx {.threadvar.}: ptr Context |
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)
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! |
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:
Changes
libwaku
user-space-main-thread with the Waku Node thread throughout channels.libwaku
user won't need to callwaku_init
norwaku_poll
.Issue
#1632