Skip to content

CA-406770: Improve error message #6537

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BengangY
Copy link
Contributor

  1. WLB request will raise wlb_authentication_failed when catching Http_client.Http_error. But actually only error code 401 and 403 should raise this type exception. For other error code, raise wlb_connection_reset. Also print the detail error code and message.
  2. message_forwarding raises same error for Http_request_rejected and Connection_reset so we don't know which exception actually be raised. Print detailed logs for these 2 exceptions.

1. WLB request will raise `wlb_authentication_failed` when catching
   `Http_client.Http_error`. But actually only error code 401 and 403
   should raise this type exception. For other error code, raise
   `wlb_connection_reset`. Also print the detail error code and
   message.
2. `message_forwarding` raises same error for `Http_request_rejected`
   and `Connection_reset` so we don't know which exception actually
   be raised. Print detailed logs for these 2 exceptions.

Signed-off-by: Bengang Yuan <[email protected]>
@BengangY BengangY marked this pull request as ready for review June 16, 2025 12:04
Comment on lines -146 to +168
| Xmlrpc_client.Connection_reset | Http_client.Http_request_rejected _ ->
warn
"Caught Connection_reset when contacting host %s; converting into \
| Xmlrpc_client.Connection_reset ->
error
"%s: Caught Connection_reset when contacting host %s; converting into \
CANNOT_CONTACT_HOST"
(Ref.string_of host) ;
__FUNCTION__ (Ref.string_of host) ;
raise
(Api_errors.Server_error
(Api_errors.cannot_contact_host, [Ref.string_of host])
)
| Http_client.Http_request_rejected s ->
error
"%s: Caught Http_request_rejected (%s) when contacting host %s; \
converting into CANNOT_CONTACT_HOST"
__FUNCTION__ s (Ref.string_of host) ;
raise
(Api_errors.Server_error
(Api_errors.cannot_contact_host, [Ref.string_of host])
)
| Xmlrpc_client.Stunnel_connection_failed ->
warn
"Caught Stunnel_connection_failed while contacting host %s; converting \
into CANNOT_CONTACT_HOST"
(Ref.string_of host) ;
error
"%s: Caught Stunnel_connection_failed while contacting host %s; \
converting into CANNOT_CONTACT_HOST"
__FUNCTION__ (Ref.string_of host) ;
Copy link
Member

Choose a reason for hiding this comment

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

I think all three can be condensed into one:

Suggested change
| Xmlrpc_client.Connection_reset | Http_client.Http_request_rejected _ ->
warn
"Caught Connection_reset when contacting host %s; converting into \
| Xmlrpc_client.Connection_reset ->
error
"%s: Caught Connection_reset when contacting host %s; converting into \
CANNOT_CONTACT_HOST"
(Ref.string_of host) ;
__FUNCTION__ (Ref.string_of host) ;
raise
(Api_errors.Server_error
(Api_errors.cannot_contact_host, [Ref.string_of host])
)
| Http_client.Http_request_rejected s ->
error
"%s: Caught Http_request_rejected (%s) when contacting host %s; \
converting into CANNOT_CONTACT_HOST"
__FUNCTION__ s (Ref.string_of host) ;
raise
(Api_errors.Server_error
(Api_errors.cannot_contact_host, [Ref.string_of host])
)
| Xmlrpc_client.Stunnel_connection_failed ->
warn
"Caught Stunnel_connection_failed while contacting host %s; converting \
into CANNOT_CONTACT_HOST"
(Ref.string_of host) ;
error
"%s: Caught Stunnel_connection_failed while contacting host %s; \
converting into CANNOT_CONTACT_HOST"
__FUNCTION__ (Ref.string_of host) ;
| (Xmlrpc_client.Connection_reset | Http_client.Http_request_rejected _ | Xmlrpc_client.Stunnel_connection_failed) as e ->
error
"%s: Caught %s when contacting host %s; converting into \
CANNOT_CONTACT_HOST"
__FUNCTION__ (Printexc.to_string e) (Ref.string_of host)
raise Api_errors.(Server_error (cannot_contact_host, [Ref.string_of host]))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I want to print the error string in Http_client.Http_request_rejected which may include an HTTP error code.

let http_rpc_recv_response use_fastpath error_msg fd =
  match response_of_fd ~use_fastpath fd with
  | None ->
      raise (Http_request_rejected error_msg)
  | Some response -> (
    match response.Http.Response.code with
    | ("401" | "403" | "500") as http_code ->
        raise (Http_error (http_code, error_msg))
    | "200" ->
        response
    | code ->
        raise (Http_request_rejected (Printf.sprintf "%s: %s" code error_msg))
  )

Copy link
Member

Choose a reason for hiding this comment

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

I would thinkPrintexc.to_string would include the error code as well.

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