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

Add additional unwrap code in ToStatusError() gRPC error handler #434

Conversation

krapie
Copy link
Member

@krapie krapie commented Dec 29, 2022

What this PR does / why we need it:

Add additional unwrap code in TostatusError() in grpchelper/status.go which unwrap’s error until cause is set to base inner error.

This additional code is needed to avoid errorToCode map’s hash of unhashable type runtime error which leads to server panic.

This will prevent Yorkie server(Yorkie SaaS API) to panic(service unavailable) on unexpected error triggered by user.

Below is ToStatusError()’s parameter, err’s inner stacks(errors)

error(*fmt.wrapError) *{
    	msg: "decode project info: connection(localhost:27017[-12]) incomplete read of message header: context canceled",
	err: error(go.mongodb.org/mongo-driver/mongo.CommandError) {
		Code: 0,
		Message: "connection(localhost:27017[-12]) incomplete read of message header: context canceled",
		Labels: []string len: 1, cap: 1, ["NetworkError"],
		Name: "",
		Wrapped: error(go.mongodb.org/mongo-driver/x/mongo/driver/topology.ConnectionError) *{
			ConnectionID: "localhost:27017[-12]",
			Wrapped: error(*errors.errorString) *{
				s: "context canceled"
			},
			init: false,
			message: "incomplete read of message header"
		},
		Raw: go.mongodb.org/mongo-driver/bson.Raw len: 0, cap: 0, nil
	}
}

Parameter err needs to be unwrapped to base inner error. If not, errorToCode[cause] map (maps error to gRPC status code) will get invalid key for map like errorToCode[mongo.CommandError] which will lead to server panic: runtime error: hash of unhashable type mongo.CommandError.

This runtime error will occur when parameter err has nested errors (ex: mongo.CommandError)

Which issue(s) this PR fixes:

No issue is related.

Special notes for your reviewer:

This PR started with server log provided by @hackerwins on discord thread on unexpected server down issue.

panic

Also, I'm not sure about the initial cause of this error, which makes connection(localhost:27017[-12]) incomplete read of message header: context canceled error. I'll keep trying to reproduce this issue in code ASAP.

Does this PR introduce a user-facing change?:

NONE

Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@CLAassistant
Copy link

CLAassistant commented Dec 29, 2022

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

Merging #434 (b12c8a9) into main (a558c0f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #434   +/-   ##
=======================================
  Coverage   46.96%   46.96%           
=======================================
  Files          71       71           
  Lines        5913     5913           
=======================================
  Hits         2777     2777           
  Misses       2831     2831           
  Partials      305      305           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. 👍 I looked at the relevant code a bit more.

ToStatusError receive internal errors defined in Yorkie. They seem to be two patterns.

Case 1: Convert MongoDB error into an internal error

	if err := result.Decode(&info); err != nil {
		if err == mongo.ErrNoDocuments {
			return nil, fmt.Errorf("default: %w", database.ErrProjectNotFound)
		}
	}

Case 2: Use MongoDB error as cause

	client, err := mongo.Connect(ctx)
	if err != nil {
		return nil, fmt.Errorf("connect to mongo: %w", err)
	}

The previous code seems to work only for Case 1. I think we can avoid both cases if we return the root cause like this PR.

server/grpchelper/status.go Outdated Show resolved Hide resolved
@krapie krapie force-pushed the feature/grpc-error-handler-additional-unwrap-code branch from d65f949 to b12c8a9 Compare December 30, 2022 00:02
@krapie krapie requested a review from hackerwins December 30, 2022 00:08
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

LGTM

@hackerwins hackerwins merged commit c726368 into yorkie-team:main Dec 30, 2022
@krapie krapie deleted the feature/grpc-error-handler-additional-unwrap-code branch December 30, 2022 00:29
@krapie krapie changed the title Add additional unwrap code in ToStatusError gRPC error handler Add additional unwrap code in ToStatusError() gRPC error handler Mar 14, 2023
@krapie krapie changed the title Add additional unwrap code in ToStatusError() gRPC error handler Add additional unwrap code in ToStatusError() gRPC error handler Mar 14, 2023
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