Closed bassosimone closed 6 years ago
I am reviewing this.
My checklist:
I am trying to build this, but I get the following error:
../include/private/common/compat.hpp:10:2: error: "Too old version of measurement-kit"
#error "Too old version of measurement-kit"
^
../include/private/common/compat.hpp:19:2: warning: "This addon will not work reliably with
measurement-kit < v0.9.0" [-W#warnings]
#warning "This addon will not work reliably with measurement-kit < v0.9.0"
^
../include/private/common/compat.hpp:22:41: error: unknown type name 'Var'
template <typename T> using SharedPtr = Var<T>;
^
../include/private/common/compat.hpp:22:44: error: expected ';' after alias declaration
template <typename T> using SharedPtr = Var<T>;
I have installed measurement-kit 0.9.0-dev by having run make install
from inside of the measurement-kit repo directory.
I pushed some commits fixing some of the things I suggested
@hellais For the records, regarding the error you had: basically I recently added to MK macros to precisely assess the version and those are only in v0.7.10 and master (aka v0.9.0-dev). The compiler MK you had was based off master (hence v0.9.0-dev) but was missing these specific macros.
So I think I have managed to wrap my head around this, thanks to useful explanations from @bassosimone on slack.
Here is what I gathered:
UvAsyncCtx
manages the lifecycle of uv_async_t
. The two core functions called are resume
and suspend
. suspend
and resume
are basically used to "transport" the callbacks in node code from the measurement-kit thread into the libuv thread.
suspend
is called inside of NettestWrap
whenever you need to have a callback called inside of the libuv thread. Then resume
will be called by the libuv event loop by means of the uv_async_send(&self->async)
.
Mapping of resume
call to the libuv event loop, happens in the make()
call of UvAsyncCtx
, by calling:
uv_async_init(uv_default_loop(), &self->async, mkuv_resume)
We don't keep uv_async_t
in the nettest class for various reasons, one being that it may have a different lifecycle to the nettest class lifecycle.
For me this PR is good to merge.
@bassosimone asked me on slack:
do you find the pattern where an async class has only static methods with shared_pointer
self
first argument more clear and less surprising than other patterns I used in the past?
I don't expect to be able to carefully think about this today, so you should either proceed with merge (and maybe make the changes I requested if you think they are appropriate) or wait for me to do another pass answering this question too.
[...] wait for me to do another pass answering this question too.
I think there's no need to rush. Let's wait! ⏸
With 301996b, these bindings have been updated to require MK >= 0.7.11. In such version of MK, I have fixed the way in which tests are run so that it works correctly with the nodejs bindings!
@bassosimone as I wrote on slack:
do you find the pattern where an async class has only static methods with shared_pointer self first argument more clear and less surprising than other patterns I used in the past?
As you probably know I am a big fan of classes, so I do find it a bit unusual that you follow a pattern of keeping a reference to the class instance and then call static methods by passing self into it I probably would have done it differently Either:
I am going to proceed with merging the PR and pushing to npm a measurement-kit release so I can start messing around with it in ooniprobe-cli
This commit introduces asynchronous node bindings that work [*]. It has been tested on Linux and macOS with MK v0.9.0-dev and v0.7.10.
It has issues with MK v0.7.10. Specifically, the main problem seems to be that tests don't terminate properly closing the libuv main loop.
This is based on #2 started by @hellais. I squashed all the commits together. For the full history, see #2.
[*] Exceptions may exist! Consult a laywer! If in doubt, don't breathe!