Skip to content
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

worker: support more cases when (de)serializing errors #47925

Merged
merged 6 commits into from
May 12, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented May 8, 2023

extracted from #47867:
the motivation of this change is for the test runner (which runs across multiple processes) to communicate with the main process via v8 serialization instead of TAP - so we want to better serialize a wider range of (edge?) cases

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 8, 2023
@MoLow MoLow force-pushed the serialize-more-error-types branch from 1ce8d41 to 868e0fb Compare May 8, 2023 18:38
@MoLow
Copy link
Member Author

MoLow commented May 8, 2023

CC @nodejs/workers @nodejs/test_runner

- error.cause is potentially an error, so is now handled recursively
- best effort to serialize thrown symbols
- handle thrown object with custom inspect
@MoLow MoLow force-pushed the serialize-more-error-types branch from 868e0fb to 640f32b Compare May 8, 2023 18:50
lib/internal/error_serdes.js Outdated Show resolved Hide resolved
@MoLow MoLow requested review from aduh95 and jasnell May 10, 2023 18:44
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests with Object.defineProperty(new Error, "cause", { get(){})? When the error cause is defined as a getter on the instance rather than the prototype?

test/parallel/test-error-serdes.js Outdated Show resolved Hide resolved
aduh95

This comment was marked as duplicate.

@MoLow MoLow requested review from aduh95 and cjihrig May 11, 2023 06:25
lib/internal/error_serdes.js Show resolved Hide resolved
lib/internal/error_serdes.js Show resolved Hide resolved
lib/internal/error_serdes.js Outdated Show resolved Hide resolved
test/parallel/test-error-serdes.js Outdated Show resolved Hide resolved
@MoLow MoLow requested a review from benjamingr May 12, 2023 06:01
@MoLow MoLow added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2023
@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 12, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 12, 2023
@nodejs-github-bot nodejs-github-bot merged commit 7984af6 into nodejs:main May 12, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 7984af6

targos pushed a commit that referenced this pull request May 14, 2023
- error.cause is potentially an error, so is now handled recursively
- best effort to serialize thrown symbols
- handle thrown object with custom inspect

PR-URL: #47925
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request May 15, 2023
- error.cause is potentially an error, so is now handled recursively
- best effort to serialize thrown symbols
- handle thrown object with custom inspect

PR-URL: #47925
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
- error.cause is potentially an error, so is now handled recursively
- best effort to serialize thrown symbols
- handle thrown object with custom inspect

PR-URL: #47925
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
- error.cause is potentially an error, so is now handled recursively
- best effort to serialize thrown symbols
- handle thrown object with custom inspect

PR-URL: nodejs#47925
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MoLow MoLow deleted the serialize-more-error-types branch May 24, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants