feat(c-bindings): add function to dealloc nodes #2499
Conversation
richard-ramos
requested review from
gabrielmer,
NagyZoltanPeter and
Ivansete-status
|
You can find the image built from this PR at Built from 6a94ea2 |
richard-ramos
force-pushed
the
waku-destroy
branch
from
2921a1e
to
7ef1293
Compare
| running.store(false) | ||
| let fireRes = ctx.reqSignal.fireSync() |
There was a problem hiding this comment.
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
And it will exit the loop in
nwaku/library/waku_thread/waku_thread.nim
Line 55 in 9ef2ecc
Because of running.load not being true anymore?
There was a problem hiding this comment.
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.
Thanks for the explanation!
I personally like that alternative better :) Find it more predictable and easier to understand
| @@ -100,6 +100,21 @@ proc waku_new(configJson: cstring, | |||
|
|
|||
| return ctx | |||
|
|
|||
| proc waku_destroy(ctx: ptr Context, | |||
There was a problem hiding this comment.
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.
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.
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.
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.
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_destroyfunction 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