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

proposal: runtime: add parameters to recover to only return specific types #50424

Closed
letian0805 opened this issue Jan 4, 2022 · 19 comments
Closed

Comments

@letian0805
Copy link

letian0805 commented Jan 4, 2022

The recover() function supports parameters in order to recover only expected exceptions.
For example:

defer func(){
	if err := recover(&MyError{}, &HelloError{}); err != nil {
		switch e := err.(type) {
		case *MyError:
			fmt.Println(e)
		case *HelloError:
			fmt.Println(e)
		}
	}
}

For now, we can handle it like this temporarily:

func Recover(expect ...interface{}) interface{} {
	if err := recover(); err != nil {
		if len(expect) == 0 {
			return err
		}
		rv1 := reflect.Indirect(reflect.ValueOf(err))
		for _, e := range expect {
			rv2 :=  reflect.Indirect(reflect.ValueOf(e))
			if rv1.Type() == rv2.Type() {
				return err
			}
		}
		panic(err)
	}
	return nil
}

However, each time Recover panic will increase the stack depth by 2. Therefore, a better way is to recover the specified exception by the runtime's recover function support parameters.

@gopherbot gopherbot added this to the Proposal milestone Jan 4, 2022
@letian0805 letian0805 changed the title proposal: affected/package: runtime proposal: affected/package: runtime, The recover function supports parameters in order to recover only expected exceptions Jan 4, 2022
@letian0805 letian0805 changed the title proposal: affected/package: runtime, The recover function supports parameters in order to recover only expected exceptions proposal: runtime: The recover function supports parameters in order to recover only expected exceptions Jan 4, 2022
@davecheney
Copy link
Contributor

davecheney commented Jan 4, 2022

It sounds like you’re trying to implement try/catch style exception handling in Go.

@letian0805
Copy link
Author

letian0805 commented Jan 4, 2022

@davecheney Yes, but without breaking the syntax of Go.

@bcmills
Copy link
Member

bcmills commented Jan 4, 2022

Compare #47653 (comment), #28150.

@ianlancetaylor ianlancetaylor changed the title proposal: runtime: The recover function supports parameters in order to recover only expected exceptions proposal: runtime: add parameters to recover to only return specific types Jan 4, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 4, 2022
@carlmjohnson
Copy link
Contributor

carlmjohnson commented Jan 5, 2022

ISTM, this could basically almost be done as user function (baring bcmills's clever trick in #47653 (comment)), except that you can't quite rethrow the same panic as before if it turns out you didn't want to recover something. Why not fix that problem instead of adding a specific mechanism for types? Maybe have panicVal, trace := catch() and throw(panicVal, trace) or some other bikeshedded names. I like that panic now is type agnostic, and I don't think it's good to make a mechanism that only lets you distinguish by types and not by value.

@letian0805
Copy link
Author

letian0805 commented Jan 5, 2022

ISTM, this could basically almost be done as user function (baring bcmills's clever trick in #47653 (comment)), except that you can't quite rethrow the same panic as before if it turns out you didn't want to recover something. Why not fix that problem instead of adding a specific mechanism for types? Maybe have panicVal, trace := catch() and throw(panicVal, trace) or some other bikeshedded names. I like that panic now is type agnostic, and I don't think it's good to make a mechanism that only lets you distinguish by types and not by value.

@carlmjohnson Why not? Usually we have our own definition of error types, which are different from runtime exceptions.And recover(except ...interface{}) is compatible with existing code.

@beoran
Copy link

beoran commented Jan 5, 2022

The fact that even the standard library uses this kind of wrappers is good evidence that this is a good idea. It is also very simple and backwards compatible. I like it. What exactly is there against this idea?

@carlmjohnson
Copy link
Contributor

carlmjohnson commented Jan 5, 2022

I would also be fine with recover(true) returning two values and panic() accepting an optional trace. I specifically don't want types to be the sole mechanism for determining what to recover because it is very plausible that one would have a type like

type mypanic struct {
  ShouldRethrow bool
  Value any
}

Just being able to catch a particular type is fine, but what is really needed is the ability to say if val.ShouldRethrow { panic(val, trace) }.

In general, there is nothing I can think of in Go's core that work only in a type specific way as is proposed in this issue. make and new of course create specific types, but that's different from filtering inputs based on their type. errors.As and errors.Is aren't in Go's core but do work sort of like this (although they have carve outs for .As() and .Is() methods, so they aren't actually type specific if you create override methods), but in those cases you can do

type MyError struct {
   error
   ShouldFrobinicate bool
}

if myerr := MyError{}; errors.As(err, &myerr) && myerr.ShouldFrobinicate {
 // use myerr because it should be frobinicated …
}

// just use err as normal, even if it was a MyError…

So, my vote FWIW is to create a more general mechanism instead of a type specific one.

@CAFxX
Copy link
Contributor

CAFxX commented Apr 15, 2022

I would also tendentially prefer panicVal, trace := recover()/panic(panicVal, trace) as a more general solution, but I am wondering what would happen in this case:

// goroutine 1
if r, trace := recover(); r != nil {
  panicCh <- struct{any, traceType}{r, trace}
}

// goroutine 2
r, trace := <-panicCh
panic(r, trace)

(or, more in general, any time the re-panic happens in a different function from the corresponding recover)

I see this as potentially useful in libraries, but I'm also worried it would make debugging somewhat more confusing if the locations of the recover and of the re-panic were not also captured in the resulting panic stack trace.

@mpx
Copy link
Contributor

mpx commented May 6, 2022

Rewriting the trace could hide significant recovery attempt logic from the traceback. Currently all re-thrown panics are visible in tracebacks.

Separately, I don't think capturing and replaying a trace via panic elsewhere should be allowed. Tracebacks are traditionally/conceptually related to a single callstack. Allowing splicing traces across separate call chains or goroutines mostly seems like it would confuse rather than aid understanding and troubleshooting. The more general approach probably isn't more generally useful.

Specifying recovery types up front:

  • avoids the complexity of replaying traces and ensures every successful recover and it's processing logic is visible via the traceback if the goroutine panics again
  • would help troubleshooting the existing practice of re-throwing panics when the type doesn't match. The added panic + defer func frames cause extra confusion at the moment and are irrelevant to actual bug(s).

@rsc
Copy link
Contributor

rsc commented Aug 12, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Aug 17, 2022

This would be a fundamental change to the language that we are unlikely to make without very significant justification, which I don't see here.

But I am intrigued by:

However, each time Recover panic will increase the stack depth by 2

What is the problem with increasing the stack depth by 2? That makes clearer what is going on in the stack trace and doesn't seem like it hurts anything.

@letian0805
Copy link
Author

letian0805 commented Aug 18, 2022

This would be a fundamental change to the language that we are unlikely to make without very significant justification, which I don't see here.

But I am intrigued by:

However, each time Recover panic will increase the stack depth by 2

What is the problem with increasing the stack depth by 2? That makes clearer what is going on in the stack trace and doesn't seem like it hurts anything.

Thank you for your attention!
Why do we need to do recovery by type? The main reason is that the project will reference many standard packages and third-party packages. Some of these packages will actively panic. For upper-level business logic, some panics do not need to exit the program, and some do. However, directly using the current native panic and recover will cause the call stack depth to increase by 2. If the level is deeper, it is very unfavorable to find the direct cause of the panic. If the recover function supports variable-length parameters, it has no effect on the existing syntax and can solve the problem.

@beoran
Copy link

beoran commented Aug 18, 2022

@letian0805 I see, there are too many irrelevant entries in the stack trace. Furthermore if there are a lot of recursive recover calls this could contribute to stack overflow problems.

@letian0805
Copy link
Author

letian0805 commented Aug 18, 2022

@letian0805 I see, there are too many irrelevant entries in the stack trace. Furthermore if there are a lot of recursive recover calls this could contribute to stack overflow problems.

@beoran Yes, developers want to be able to quickly see which line is panic, and don't want to see a lot of useless information from top to bottom.

@rsc
Copy link
Contributor

rsc commented Sep 7, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@mpx
Copy link
Contributor

mpx commented Sep 9, 2022

What is the problem with increasing the stack depth by 2? That makes clearer what is going on in the stack trace and doesn't seem like it hurts anything.

I find these extra panic frame obfuscate the real issues and make troubleshooting harder. Yes, they represent the actual logic, but that's only since Go is unable to express the logic actually desired (only recover for a specific known type). If Go was able to catch a specific type (eg an internal parser error), it would result in much cleaner stack traces when some other panic occurs.

@jimmyfrasche
Copy link
Member

jimmyfrasche commented Sep 9, 2022

If this existed I would sometimes use it to catch only a special error type but every other time I used recover I'd write recover(error) so I didn't have to worry about the awkward case of recovering a type without an Error method.

@rsc
Copy link
Contributor

rsc commented Sep 21, 2022

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests