-
Notifications
You must be signed in to change notification settings - Fork 50
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(c-bindings): add function to dealloc nodes #2499
Conversation
You can find the image built from this PR at
Built from 6a94ea2 |
2921a1e
to
7ef1293
Compare
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.
Thanks so much!
running.store(false) | ||
let fireRes = ctx.reqSignal.fireSync() |
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.
Please let me know if I understood correctly.
If we fire the signal, the thread moves from
nwaku/library/waku_thread/waku_thread.nim
Line 59 in 9ef2ecc
waitFor ctx.reqSignal.wait() |
And it will exit the loop in
nwaku/library/waku_thread/waku_thread.nim
Line 55 in 9ef2ecc
while running.load == true: |
Because of running.load
not being true
anymore?
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.
Yes, it should move from waitFor ctx.reqSignal.wait()
and stop in the next line:
let recvOk = ctx.reqChannel.tryRecv(request)
recvOk
is false at this point, so the iteration will end and next loop will not be executed due to running.load
being false.
I'm actually not sure if tryRecv
will always be false. Perhaps I should change the code so it's like this? let me know what you think:
while running.load == true:
## Trying to get a request from the libwaku main thread
var request: ptr InterThreadRequest
waitFor ctx.reqSignal.wait()
if not running.load: break <------------------------------
let recvOk = ctx.reqChannel.tryRecv(request)
if recvOk == true:
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.
Thanks for the explanation!
I personally like that alternative better :) Find it more predictable and easier to understand
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!
Was just thinking of if we can automate this destroy thread process...
@@ -100,6 +100,21 @@ proc waku_new(configJson: cstring, | |||
|
|||
return ctx | |||
|
|||
proc waku_destroy(ctx: 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.
Is it the lib user responsibility to call this?
Is there a chance to add an exit handler that can automatically do it?
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.
Yes. Bindings in other languages can automate the call to this function to be executed on some destructor function if available.
Do you have an example of the exit handler idea? I'm not sure how to implement this, so look at something i could use as a guide.
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! Thanks! 💯
Just added some minor comments :)
library/libwaku.nim
Outdated
let stopNodeRes = waku_thread.stopWakuNodeThread(ctx) | ||
if stopNodeRes.isErr(): | ||
let msg = $stopNodeRes.error | ||
callback(RET_ERR, unsafeAddr msg[0], cast[csize_t](len(msg)), userData) | ||
return RET_ERR |
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 isOkOr
and valueOr
templates from the stew module as super handy. In this case, we can reduce it a little bit:
let stopNodeRes = waku_thread.stopWakuNodeThread(ctx) | |
if stopNodeRes.isErr(): | |
let msg = $stopNodeRes.error | |
callback(RET_ERR, unsafeAddr msg[0], cast[csize_t](len(msg)), userData) | |
return RET_ERR | |
(waku_thread.stopWakuNodeThread(ctx)).isOkOr: | |
let msg = $error | |
callback(RET_ERR, unsafeAddr msg[0], cast[csize_t](len(msg)), userData) | |
return RET_ERR |
library/waku_thread/waku_thread.nim
Outdated
@@ -95,12 +95,16 @@ proc createWakuThread*(): Result[ptr Context, string] = | |||
|
|||
return ok(ctx) | |||
|
|||
proc stopWakuNodeThread*(ctx: ptr Context) = | |||
proc stopWakuNodeThread*(ctx: ptr Context): Result[void, string] = |
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.
Out of the scope of this PR but I just realised it worth keeping consistency with the "start" proc:
proc stopWakuNodeThread*(ctx: ptr Context): Result[void, string] = | |
proc stopWakuThread*(ctx: ptr Context): Result[void, string] = |
Description
While working with the rust bindings I noticed that when I closed the example program, it segfaulted, and this happened due to the waku thread still being alive. This PR adds a
waku_destroy
function that stops the thread, as well as firing a signal so the code does not stay stuck atnwaku/library/waku_thread/waku_thread.nim
Line 59 in 9ef2ecc