Skip to content

Conversation

skipkayhil
Copy link
Contributor

This was previously removed when error classes were refactored, but it doesn't seem to have been done so intentionally.

When debugging a previous fix, this missing context would have helped narrow down the problem initially as the rbmsg includes where the error was originating.

@skipkayhil
Copy link
Contributor Author

skipkayhil commented May 13, 2024

I'm also curious if these lines in auth_switch should be changed to trilogy_auth_switch_recv

handle_trilogy_error(ctx, rc, "trilogy_auth_recv");
}
rc = trilogy_sock_wait_read(ctx->conn.socket);
if (rc != TRILOGY_OK) {
handle_trilogy_error(ctx, rc, "trilogy_auth_recv");

While there is no trilogy_auth_switch_recv method, making the rbmsg here different than the one in authenticate would also be helpful for tracking down the line raising an error.

This was previously [removed][1] when error classes were refactored, but
it doesn't seem to have been done so intentionally.

When debugging a previous [fix][2], this missing context would have
helped narrow down the problem initially as the rbmsg includes where the
error was originating.

[1]: f9ee855
[2]: cb788be
@skipkayhil skipkayhil force-pushed the hm-restore-error-context branch from e9a5d6a to 25aa20b Compare July 23, 2024 15:41
Copy link
Collaborator

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

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

Thanks, yeah don't think we intended to remove this! 😅

I'm also curious if these lines in auth_switch should be changed to trilogy_auth_switch_recv

I think we should still reference the call that failed, but maybe we could add more context to the message, like trilogy_auth_recv on authenticate vs trilogy_auth_recv on auth_switch?

@adrianna-chang-shopify adrianna-chang-shopify merged commit e84501b into trilogy-libraries:main Jul 23, 2024
11 checks passed
@skipkayhil skipkayhil deleted the hm-restore-error-context branch July 23, 2024 16:02
bquorning added a commit to zendesk/active_record_host_pool that referenced this pull request Dec 9, 2024
A Trilogy error message needed adjusting, see
trilogy-libraries/trilogy#187
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.

2 participants