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.

Sign up for GitHub

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

Jump to bottom

feat(c-bindings): add function to dealloc nodes #2499

Merged
merged 3 commits into from
Mar 7, 2024
Merged

feat(c-bindings): add function to dealloc nodes #2499

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ef1293
feat(c-bindings): add function to dealloc nodes
richard-ramos Mar 4, 2024
a4cf1ac
fix: code review
richard-ramos Mar 6, 2024
1adfb48
fix: code review pt2
richard-ramos Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions library/libwaku.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ int waku_stop(void* ctx,
WakuCallBack callback,
void* userData);

// Destroys an instance of a waku node created with waku_new
int waku_destroy(void* ctx,
WakuCallBack callback,
void* userData);

int waku_version(void* ctx,
WakuCallBack callback,
void* userData);
Expand Down
15 changes: 15 additions & 0 deletions library/libwaku.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,21 @@ proc waku_new(configJson: cstring,

return ctx

proc waku_destroy(ctx: 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.

Is it the lib user responsibility to call this?
Is there a chance to add an exit handler that can automatically do it?

Copy link
Member Author

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.

callback: WakuCallBack,
userData: pointer): cint {.dynlib, exportc.} =

if isNil(callback):
return RET_MISSING_CALLBACK

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
Copy link
Collaborator

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:

Suggested change
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


return RET_OK

proc waku_version(ctx: ptr Context,
callback: WakuCallBack,
userData: pointer): cint {.dynlib, exportc.} =
Expand Down
8 changes: 6 additions & 2 deletions library/waku_thread/waku_thread.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ proc run(ctx: ptr Context) {.thread.} =
var request: ptr InterThreadRequest
waitFor ctx.reqSignal.wait()
let recvOk = ctx.reqChannel.tryRecv(request)
if recvOk == true:
if running.load and recvOk == true:
richard-ramos marked this conversation as resolved.
Show resolved Hide resolved
let resultResponse =
waitFor InterThreadRequest.process(request, addr node)

Expand Down Expand Up @@ -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] =
Copy link
Collaborator

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:

Suggested change
proc stopWakuNodeThread*(ctx: ptr Context): Result[void, string] =
proc stopWakuThread*(ctx: ptr Context): Result[void, string] =

running.store(false)
let fireRes = ctx.reqSignal.fireSync()
Copy link
Contributor

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?

Copy link
Member Author

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:

Copy link
Contributor

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

if fireRes.isErr():
return err(fireRes.error)
richard-ramos marked this conversation as resolved.
Show resolved Hide resolved
joinThread(ctx.thread)
discard ctx.reqSignal.close()
discard ctx.respSignal.close()
freeShared(ctx)
ok()
richard-ramos marked this conversation as resolved.
Show resolved Hide resolved

proc sendRequestToWakuThread*(ctx: ptr Context,
reqType: RequestType,
Expand Down
Loading