-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Fix improper handling of msg.results in chunk callbacks (#964) #1076
base: master
Are you sure you want to change the base?
Fix improper handling of msg.results in chunk callbacks (#964) #1076
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate why wee need to terminate the worker only when a function is defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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
complete
when a chunk callback was present.To address this, the release point of
msg.results
has been adjusted to ensure that results are properly passed tocomplete
in cases where a chunk callback is used.