-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Canvas] Fix: avoid server crash when no value is found #29069
Conversation
💚 Build Succeeded |
worker.send({ type: 'msgSuccess', id, value: value }); | ||
}) | ||
.catch(e => heap[threadId].reject(e)); | ||
if (heap[threadId]) { |
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.
how is it possible that the heap[threadId] does not exist ?
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 don't fully grok it at the moment, as mentioned in chat I'd like to learn more about the heap
concept here, will reach out to @w33ble who's most familiar with the code.
The error was caused by acting on an object that isn't in this lookup table, so it's safe to avoid that and with that, the server crash.
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.
This came up in one of Peter's PRs too, but he thought at the time it was related to some other change. I think @chrisdavies did an awesome job explaining what happens in this comment
What's happening is that
worker.js
is getting lots of calls toonFunctionNotFound
, each with the samethreadId
, but with different params
This happens because something like mapColumn
will map over every row, as you can see in the example Stacey provided in the original issue
Once the first one fails, the
threadId
is removed from the heap inthread/index.js
, so the subsequent calls fail to find any heap context for thatthreadId
.
And that's why it was blowing up. In Stacey's example, she was using doesntexist
(an invalid function) as the function to run in mapColumn
. That rejects, clears the heap, and subsequent heap actions blow up because the id is no longer valid.
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.
LGTM. This does fix the Node crash, but as mentioned, it's a stop-gap until we can figure out how it's happening. I have several hunches that I'm going to look into.
Summary
Closes #29035