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

Send payload id if applicable when an exception is caught #545

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

o0Ignition0o
Copy link
Contributor

If an exception is caught during planning, we previously didn't send the messageId.

The response could thus not be properly routed back to the proper sender, and would lead to such errors as:

value: TypeError: Error parsing args at position 1: missing field `id`

This changeset adds the messageId if possible.

@o0Ignition0o o0Ignition0o requested review from a team as code owners July 19, 2024 10:48
Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

Hmm. It looks like this could only occur if receive() errored? I'm not clear on when that would happen (though this patch would surely help to figure that out 😄 )

I wonder if we should accept Option<String> on the rust side rather than using a default empty value? Or is that super invasive?

router-bridge/js-src/plan_worker.ts Outdated Show resolved Hide resolved
Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

Ohh wait. I reread it after. My comment about receive() is incorrect.

This patch looks good, we would only get the default empty value if receive() fails, which should not happen ( 😇 ), and we now get the correct value if QP fails, which we previously didn't.

Adjusting my question: is getting an empty value going to be a problem in the router? It shouldn't happen, but it might ;p Either way this is better than before.

@duckki
Copy link

duckki commented Jul 24, 2024

This seems working. I don't see this error with this PR.

@o0Ignition0o o0Ignition0o merged commit feca445 into main Jul 29, 2024
13 checks passed
@o0Ignition0o o0Ignition0o deleted the igni/send_id_on_error branch July 29, 2024 08:22
@o0Ignition0o o0Ignition0o restored the igni/send_id_on_error branch July 29, 2024 08:22
@o0Ignition0o o0Ignition0o deleted the igni/send_id_on_error branch July 29, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants