-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(core): add option to use v8 for daemon message serialization to avoid issues when JSON.stringify would fail #30516
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
View your CI Pipeline Execution ↗ for commit 7a66080.
☁️ Nx Cloud last updated this comment at |
91f6d67 to
4c8bfff
Compare
🐳 We have a release for that!This PR has a release associated with it. You can try it out using this command: npx [email protected] my-workspaceOr just copy this version and use it in your own command: 0.0.0-pr-30516-4c8bfff
To request a new release for this pull request, mention someone from the Nx team or the |
4c8bfff to
94d09ba
Compare
🐳 We have a release for that!This PR has a release associated with it. You can try it out using this command: npx [email protected] my-workspaceOr just copy this version and use it in your own command: 0.0.0-pr-30516-94d09ba
To request a new release for this pull request, mention someone from the Nx team or the |
9fc6183 to
3f73f1a
Compare
3f73f1a to
ff3e197
Compare
|
|
||
| // Server may send multiple messages in one chunk, so splitting by 0x4 | ||
| const messages = message.split(''); | ||
| const messages = message.split(MESSAGE_END_SEQ); |
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.
There appears to be a logical issue with the message splitting implementation. The code first removes MESSAGE_END_SEQ from the end of the chunk, then attempts to split the resulting message by MESSAGE_END_SEQ. However, since the end sequence has already been removed, this split operation won't find any delimiters in the current message.
To properly handle multiple messages in one chunk, consider either:
- Split the original chunk by
MESSAGE_END_SEQbefore removing the end sequence, or - Remove this split operation entirely if the design only expects one message per chunk after removing the end sequence
This could lead to missed messages if multiple complete messages arrive in a single chunk.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| // strings | ||
| (message.startsWith('"') && message.endsWith('"')) || | ||
| // numbers | ||
| /^[0-9]+(\\.?[0-9]+)?$/.test(message) |
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.
The regex pattern for matching numbers has an issue with the escaped decimal point. The current pattern /^[0-9]+(\.?[0-9]+)?$/ makes both the decimal point and the digits after it optional, which would incorrectly match strings like "123." (ending with a dot).
A more accurate pattern would be /^[0-9]+(\.[0-9]+)?$/ where only the entire decimal portion (dot + digits) is optional, ensuring that if a decimal point is present, it must be followed by at least one digit.
| /^[0-9]+(\\.?[0-9]+)?$/.test(message) | |
| /^[0-9]+(\\.?[0-9]+)?$/.test(message) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
…void issues when JSON.stringify would fail fix(core): v8 serializer should be able to hash tasks feat(core): use v8 serializer for daemon messages by default
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
ff075d9 to
0e1173a
Compare
| `No pending promise found for transaction "${tx}". This may indicate a bug in the plugin pool. Currently pending promises:\n` + | ||
| Object.keys(pending).map((t) => ` - ${t}`).join('\n') |
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.
There's an issue with the error message construction. Since pending is a Map, using Object.keys(pending) won't work correctly. To properly display the keys from the Map, use Array.from(pending.keys()) instead:
`No pending promise found for transaction "${tx}". This may indicate a bug in the plugin pool. Currently pending promises:\n` +
Array.from(pending.keys()).map((t) => ` - ${t}`).join('\n')| `No pending promise found for transaction "${tx}". This may indicate a bug in the plugin pool. Currently pending promises:\n` + | |
| Object.keys(pending).map((t) => ` - ${t}`).join('\n') | |
| `No pending promise found for transaction "${tx}". This may indicate a bug in the plugin pool. Currently pending promises:\n` + | |
| Array.from(pending.keys()).map((t) => ` - ${t}`).join('\n') |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
…void issues when JSON.stringify would fail (#30516) <!-- Please make sure you have read the submission guidelines before posting an PR --> <!-- https://github.com/nrwl/nx/blob/master/CONTRIBUTING.md#-submitting-a-pr --> <!-- Please make sure that your commit message follows our format --> <!-- Example: `fix(nx): must begin with lowercase` --> <!-- If this is a particularly complex change or feature addition, you can request a dedicated Nx release for this pull request branch. Mention someone from the Nx team or the `@nrwl/nx-pipelines-reviewers` and they will confirm if the PR warrants its own release for testing purposes, and generate it for you if appropriate. --> ## Current Behavior For really large objects (particularly those containing large strings) JSON.stringify can fail ## Expected Behavior Daemon serialization doesn't fail for the same strings when setting `NX_USE_V8_SERIALIZER=true`. This should become the default behavior. ## Related Issue(s) <!-- Please link the issue being fixed so it gets closed when this is merged. --> Fixes # --------- Co-authored-by: Jason Jean <[email protected]> Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com> (cherry picked from commit a59e6eb)
|
This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request. |
Current Behavior
For really large objects (particularly those containing large strings) JSON.stringify can fail
Expected Behavior
Daemon serialization doesn't fail for the same strings when setting
NX_USE_V8_SERIALIZER=true. This should become the default behavior.Related Issue(s)
Fixes #