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

Handle function call errors more gracefully in routes #24120

Closed
wxiaoguang opened this issue Apr 14, 2023 · 5 comments
Closed

Handle function call errors more gracefully in routes #24120

wxiaoguang opened this issue Apr 14, 2023 · 5 comments
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 14, 2023

Feature Description

Example: help more problems like #24118

Before

	count, err := actions_model.CountRunners(ctx, opts)
	if err != nil {
		ctx.ServerError("AdminRunners", err)
		return
	}

There are some problems:

  1. The function names changes during refactoring, so the ServerError frequently gets out-of-sync.
  2. People have to copy&paste duplicate code again and again.
  3. The return could be forgotten, a lot of times.

Proposal 1

Use generic, let a function like ErrorProof handles server errors automatically.

ErrorProof can use reflect to get the target function's name, it doesn't affect performance because in most cases there is no "internal server error".

	count, err := ctx.ErrorProof(actions_model.CountRunners, ctx, opts))
	if err != nil {
		return
	}

Proposal 2

Return the err directly, make our handler framework support such return value for handler functions (with stacktrace):

	count, err := actions_model.CountRunners(ctx, opts)
	if err != nil {
		return ctx.ErrorWithStack(err)
	}

Feasible?

These are just some early ideas, haven't tried whether it is really feasible, but if people like this proposal, I will spend some time to try.

I really prefer the second one, it looks clear and intuitive

@wxiaoguang wxiaoguang added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Apr 14, 2023
@wxiaoguang wxiaoguang changed the title Handle functoin call errors more gracefully in routes Handle function call errors more gracefully in routes Apr 17, 2023
@silverwind
Copy link
Member

silverwind commented Apr 21, 2023

In a sane language with exceptions, I would say throw those errors, but given golang has no exceptions besides panic/recover (I'm not sure it should those can or should be used as a rudimentary exception system), error return is probably the next best thing.

Ideally there should be linting for this but I fear it might require us to implement this linting ourselves, or maybe some existing linters can already help.

@lunny
Copy link
Member

lunny commented May 29, 2023

I like proposal 2, I have ever implemented a web framework https://gitea.com/lunny/tango support to receive one or two return error parameters like

func a() error
or
func b() (statusCode int, error)

@wxiaoguang
Copy link
Contributor Author

I also have done similar thing in my Java/PHP/Golang web frameworks.

Usually I make the return type as an interface, then the handler could have the full control for what it wants to output.

The common responses could be provided by builtin response classes:

func handler(ctx *web.Context) Response {
    return ctx.JSON()
}

func handler(ctx *web.Context) Response {
    return ctx.HTML()
}

func handler(ctx *web.Context) Response {
    return ctx.PlainText()
}

func handler(ctx *web.Context) Response {
    return ctx.ServerError()
}

func handler(ctx *web.Context) Response {
    return &MyResponseWriter{  write status, write headers, write body ... }
}

@wolfogre
Copy link
Member

wolfogre commented Jul 5, 2023

I agree, it surprised me the first time I saw this:

	v := DoSomething(ctx)
	if ctx.Written() {
		return
	}
	// then the v is valid

@wxiaoguang
Copy link
Contributor Author

No time for it. Close.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

No branches or pull requests

4 participants