chore(cbindings): avoid using global var in libwaku.nim #2118
Conversation
* Avoid using a global variable to store the Context object. * Better signature for the callback. Two new parameters added: one aimed to allow passing the caller result code; and the other param is to pass an optional userData pointer that might need to be linked locally with the Context object. This is needed in Rust for example, to make the passed closures to live as long as the Context.
|
You can find the image built from this PR at Built from 7a34a04 |
Ivansete-status
requested review from
richard-ramos,
arnetheduck,
NagyZoltanPeter and
SionoiS
There was a problem hiding this comment.
Pretty cool step ahead! Thank you!
I think it is quite well polished overall.
I just have some notice for the future to think of but handle it as a personal flavour.
I think in general library internal memory shall not be exposed to client. I personally more like to name it as handle (e.g. ContextHandle that can be checked if that maps to a real object allocated in the library, rather believe caller gives us the right pointer back). Main reason that any kind of mess in the usage will point to library failure, like crash or something.
Similar but even harder to solve in C the thingy with returning strings through callback.
We should trust client code that our message pointer will not outlive the callback call or GC (does GC enabled in libwaku?) can free or reuse it causing weird working in the user code.
In the example program I would encourage use strncpy not to trust ever 0 ending.
Also malloc call is better "malloc((len+1)*sizeof(char))" in case sometime ever it turns to wchar_t. But this one is really for habits, not a bug in this particular case.
|
Thanks for the comment and for checking it! 🔥 I reply to a couple of points:
Good point! I'll revisit all points where we return information through the callback approach to make sure the strings are properly allocated, live during the callback call, and are freed afterwards.
Very good point indeed! Something to enhance but not urgent. |
|
|
||
| // Creates a new instance of the waku node. | ||
| // Sets up the waku node from the given configuration. | ||
| int waku_new(const char* configJson, WakuCallBack onErrCb); | ||
| int waku_new(void* ctx, |
There was a problem hiding this comment.
for the context, a useful trick is to define an opaque type for it (typedef struct WakuContext WakuContext;) and pass around WakuContext* instead of void* - this documents intent and gives some rudimentary type safety (even if it's limited in C).
* libwaku: Avoid global variable and changing callback signature * Better signature for the callback. Two new parameters have been added: one aimed to allow passing the caller result code; the other param is to pass an optional userData pointer that might need to be linked locally with the Context object. For example, this is needed in Rust to make the passed closures live as long as the Context. * waku_example.c: adaptation to the latest changes * libwaku.h: removing 'waku_set_user_data' function * libwaku.nim: renaming parameter in WakuCallBack (isOk -> callerRet)
Description
Applying Jacek's recommendation which is to prevent using global variable. That way we provide more flexibility to the library users. For example, we could start multiple
wakunodeinstances within the same app.Original recommendation: #1865 (comment)
Changes
libwaku.nim.ctxvariable to outside the library so that the Context can be passed to any other libwaku function.int callerRetparameter toWakuCallback. This parameter allows the caller to indicate whether the callback call was a successful or erroneous one.void* userDataparameter to every library function. This is useful to attach closure reference in Rust.Issue
#1878