test: Add test case for canceling async worker tasks #1202
Conversation
test/async_worker.cc
Outdated
| } | ||
|
|
||
| void OnError(const Error&) override { | ||
| NAPI_THROW_IF_FAILED_VOID(this->Env(), napi_generic_failure); |
There was a problem hiding this comment.
It can be more intuitive if we can throw an error with an explicit message about the reason here:
| NAPI_THROW_IF_FAILED_VOID(this->Env(), napi_generic_failure); | |
| Napi::Error::New(env, "Unreachable").ThrowAsJavaScriptException(); |
test/async_worker.cc
Outdated
| } | ||
|
|
||
| void OnOK() override { | ||
| NAPI_THROW_IF_FAILED_VOID(this->Env(), napi_generic_failure); |
test/async_worker.cc
Outdated
| static void DoWork(const CallbackInfo& info) { | ||
| Function cb = info[0].As<Function>(); | ||
| std::string echo = info[1].As<String>(); | ||
| for (int i = 1; i < MAX_CANCEL_THREADS; i++) { |
There was a problem hiding this comment.
Maybe we can get the cpu number with JS API and fill the worker threads with that number of workers? In that case, the test can be more stable.
With JS specified worker filling number, we can also test that cancelWorker->Cancel() throws if the cancel worker has started the work.
There was a problem hiding this comment.
Hey Legendecas! Thank you for the review. By cpu numbers do you mean the size of the Libuv's thread pool? I tested this by setting UV_THREADPOOL_SIZE to smaller numbers such as 2 or 3 and it seems to work reliably in that the CancelWorker is able to cancel the queue'd task.
Also, I am running into some difficulties testing that Cancel() throws if the worker has started the task. Initially, I just passed the value 0 as the threadNum to queue a task for the Cancel worker to cancel, expecting an error to be thrown. For some reason, this exception is only thrown once by Cancel().
After doing some investigation, it looks like we run this test using common.runTest where we run the test function using 4 different node-gyp configs in rapid succession. Could it be that by the second time the test function is invoked, the thread pool is busy and cancellation is successful? If that's the case is there a way where we could check to make sure the threads are idle before running the test suite? Thanks.
There was a problem hiding this comment.
Looks like we already have a similar test case in core (https://github.com/nodejs/node/blob/main/test/node-api/test_async/test_async.c#L140) and it has been running in a quite stable way for a long time. If that's the case, I'm fine with the current pattern as it is the simplest way to test the API.
Yeah, if the test case is mixed up with other test suites, we can not guarantee that the threadpool is ready to execute the work immediately. But I believe we can still facilitate mutexes and conditional variables to notify the JS thread when the Worker is started to execute. However, this increases the complexity of this test case and I assume it is fine to do it in a follow up PR.
|
Landed as 793268c |
Adding a small test case for Async workers'
Cancelfunctionality, where if the cancellation is successful neitherOnOkorOnErrorshould be invoked.