Fix improper handling of msg.results in chunk callbacks (#964)#1076
Fix improper handling of msg.results in chunk callbacks (#964)#1076jongwoo328 wants to merge 4 commits into
Conversation
pokoli
left a comment
pokoli
left a comment
There was a problem hiding this comment.
Thanks for submiting the PR.
Do you think it should be easy to have a test suite which tests that the chunk Callback gets proper results? It will be great to include such test to prevent any regresion on the future.
| var worker = workers[workerId]; | ||
| if (isFunction(worker.userComplete)) | ||
| worker.userComplete(results); | ||
| worker.terminate(); |
There was a problem hiding this comment.
Could you elaborate why wee need to terminate the worker only when a function is defined?
There was a problem hiding this comment.
I think I may have unintentionally changed the original flow while modifying the code. I'll revert back to. Thank you.
|
Sure. Should I add test to the node_test.js and CUSTOM_TEST in test-cases.js? |
I will wait for your test before merging this. |
|
I've addressed my mistakes and adding tests. |
I've run the test and I see your new test is failing. Could you have a look and address it? TIA |
|
Previous fixes didn't work on node environments 😅. Now it passes all tests! |
|
Hi @pokoli |
|
Hi @pokoli |
2 participants
Fix #964.
The
delete msg.results;statement was likely introduced for memory optimization.However, this caused the results to not be correctly passed as a parameter to
completewhen a chunk callback was present.To address this, the release point of
msg.resultshas been adjusted to ensure that results are properly passed tocompletein cases where a chunk callback is used.