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

spec: guarantee non-nil return value from recover #25448

Closed
bradfitz opened this issue May 17, 2018 · 89 comments
Closed

spec: guarantee non-nil return value from recover #25448

bradfitz opened this issue May 17, 2018 · 89 comments
Assignees
Labels
LanguageChange NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted v2 A language change or incompatible library change
Milestone

Comments

@bradfitz
Copy link
Contributor

Calling panic with a nil panic value is allowed in Go 1, but weird.

Almost all code checks for panics with:

     defer func() {
        if e := recover(); e != nil { ... }
     }()

... which is not correct in the case of panic(nil).

The proper way is more like:

     panicked := true
     defer func() {
        if panicked {
              e := recover()
              ...
        }
     }()
     ...
     panicked = false
     return
     ....
     panicked = false
     return

Proposal: make the runtime panic function promote its panic value from nil to something like a runtime.NilPanic global value of private, unassignable type:

package runtime

type nilPanic struct{}

// NilPanic is the value returned by recover when code panics with a nil value.
var NilPanic nilPanic

Probably Go2.

@cznic
Copy link
Contributor

cznic commented May 17, 2018

Dear runtime,

if I perform panic(nil) or panic(somethingThatCanBeNil), it may be a mistake. That's my problem. But I may also do that intentionally and with the magically changed value, I need to not forget about that and workaround the magic.

Less the magic, the less exceptional rules I have to think about. It makes me more productive and my code more comprehensible to my future self. Thanks.

edit: typo

@mdempsky
Copy link
Member

An alternative solution would be to allow recover() to be used in ,ok assignments. For that to really solve the stated problem though, that would require all call sites to be updated.

My personal leaning at the moment is in favor of the proposal as stated.

@mvdan
Copy link
Member

mvdan commented May 17, 2018

Is any program out there using nil panics intentionally? A simple search for panic(nil) doesn't give anything on my entire GOPATH besides a go/ssa/interp test. But I'm more worried about panics with variables that could/would be nil.

In any case, I agree with the sentiment and the proposed solution. Perhaps runtime.NilPanic should be clarified that it's only for untyped nils, though. For example, this case has a nil value but wouldn't be equal to nil when recovered:

var err *myError
panic(err)

@cznic
Copy link
Contributor

cznic commented May 17, 2018

FTR: I admit this is a real problem. I just prefer explicitly handling it.

@robpike
Copy link
Contributor

robpike commented May 18, 2018

Is this a real problem, though? I doubt it. What if the implementation of panic with a nil argument instead just did, panic("nil value passed to panic")? Thereby fixing the problem and diagnosing it one one stroke.

@bradfitz
Copy link
Contributor Author

Is this a real problem, though? I doubt it.

I've had to deal with it at least twice. net/http had hangs when people panicked with nil.

@mpvl was talking about error handling the other day and was showing some examples of how defensive code should ideally look like (and how it's hard to get right), and he forgot the nil panic case, showing it's even harder than it seems.

What if the implementation of panic with a nil argument instead just did, panic("nil value passed to panic")? Thereby fixing the problem and diagnosing it one one stroke.

That's what I'm proposing, except with a variable (which could have a String method with that text). But I'm fine (but less fine) with it being that string value exactly, as matching on strings is a little gross.

@rsc rsc added the v2 A language change or incompatible library change label May 21, 2018
@wgoodall01
Copy link

This proposal makes total sense--for a language which prides itself on its simplicity and obviousness, it is perplexing that the only way to check for a panic(nil) in a recover is by using some other variable. I can't possibly think up a situation when someone would call panic(nil) on purpose anyhow.

@deanveloper
Copy link

While I believe that this is a very good thing to be able to handle, I think that a , ok pattern would be a bit better. If I panic(nil) then in a language like Go, I would want nil to be the recover() value.

So then recover would look like:

defer func() {
    if e, ok := recover(); ok {
        // do recovery stuff
    }
}();

@cznic
Copy link
Contributor

cznic commented May 30, 2018

@deanveloper I like your idea for being backwards compatible.

@bcmills
Copy link
Contributor

bcmills commented Sep 10, 2018

The proper way is more like: […]

That turns out not to be correct either: runtime.Goexit can terminate the goroutine without a recoverable value, but that pattern (and the variant as written by @cznic) would incorrectly report a panic where no panic occurred.
(https://play.golang.org/p/yS1A1c5csrR)

As far as I can tell, there is no way for a defer call to distinguish between panic(nil) and runtime.Goexit(), which to me suggests that at least one of them should be removed.

@bcmills
Copy link
Contributor

bcmills commented Sep 10, 2018

As far as I can tell, there is no way for a defer call to distinguish between panic(nil) and runtime.Goexit(), which to me suggests that at least one of them should be removed.

As it turns out, we can use the fact that a runtime.Goexit() cannot be recover'd to distinguish it from panic(nil), via a “double defer sandwich”:
https://play.golang.org/p/nlYYWPRO720

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/134395 mentions this issue: errgroup: rethink concurrency patterns

@ianlancetaylor
Copy link
Contributor

I think we should treat panic(nil) as a runtime error, so it should panic with a value that implements the runtime.Error interface. This is less convenient for people who want to explicitly detect panic(nil), but I don't see why that matters; if you want to check for it, then, instead, don't do it.

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 12, 2018
@deanveloper
Copy link

if you want to check for it, then, instead, don't do it.

Perhaps you're using a badly-written library, and you don't have control over it. I've seen worse before 🤷‍♂️

@ghost
Copy link

ghost commented Apr 3, 2019

Our team spent 3 hours today hunting down what we now know to be a panic(nil) call. The problem is that panic(nil) is valid yet undetectable since recover() only returns the value that was panic-ed.

This proposal as-is would work as the common if recover() != nil { pattern would see a non-nil value. Therefore, has my vote. (Some internal type would work too, as long as it is not nil)

Making recover() return two values (the panic-ed value and whether a panic occurred) as mentioned several times would make more sense, However, changing recover() will undoubtedly be rejected for being non-backward compatible. Or too complicated if made magical (like map and range that provide one or two values, depending how you use it).

@zigo101
Copy link

zigo101 commented Apr 3, 2019

Sometimes. people may call panic(nil) for success. See the case 3 in this article for an example.

[edit] in this example, the panic value is not nil, but it can be.

@bradfitz
Copy link
Contributor Author

bradfitz commented Apr 3, 2019

@go101, eliminating code like that would be an added bonus.

@zigo101

This comment has been minimized.

@bradfitz

This comment has been minimized.

@zigo101

This comment has been minimized.

@DeedleFake
Copy link

DeedleFake commented Jan 11, 2023

It seems unlikely that code both needs compatibility with an older toolchain and needs to catch panic(nil) in certain packages written for the newer version of Go. When you update from the older toolchain to the newer toolchain, by definition you don't have any packages written for the newer toolchain yet.

The thing I'm worried about there is that some dependency of a package may make use of panic(nil) internally. It's rare, sure, but if it happens and that dependency suddenly uses the new semantics without expecting it, it could lead to horrible subtle bugs for the client of that dependency. By tying it to the go.mod file of each module instead of a global one, the dependencies can update their code and the semantics simultaneously. By tying it to GODEBUG and making it a global flag, it breaks reproducibility and makes the main module dependent on probably undocumented behavior of its dependencies.

I feel like as much as possible semantics changes should be isolated to modules and specific go <version> directives, not global flags, or otherwise there's not much point to versioning them at all, since the main module changing will suddenly break stuff internal to other modules. It's smaller scale, but it's similar to the Python 3 debacle in that it makes code dependent on updates to their dependencies, something out of their direct control. Avoiding that was a big reason that the go <version> directive was made per-module in the first place. GODEBUG is better than Python 3's approach, but it's still a big step backwards from everything I've seen.

@rsc
Copy link
Contributor

rsc commented Jan 12, 2023

@DeedleFake, both approaches have the dependency problem: if we make the fix global then you are dependent on dependencies to correct any breakages, and if we make the fix per-module then you are dependent on dependencies to update their code to get fixed behavior. We have to weigh the two sides and decide which should be preferred.

For the specific case of panic(nil), the evidence is that there is a lot of code out there subtly broken today, which this change will fix, and there is hardly any code out there depending on panic(nil) showing up as a recover of nil, which this change will break. So the balance came out in favor of a global fix.

It is worth noting that making just about any change at all to Go at this point requires that same balance, so it will almost always also motivate a global fix like this.

There is also the problem of changing behavior based on where a function call is located, which in general is expensive and confusing. For this specific case an argument can be made that recover is already aware of its caller in a very specific way, and what's the harm in one additional way. But that argument would not apply to other cases, and I am not even convinced the situation warrants it here. It certainly wouldn't be the precedent we want in general, as I explained on #56986 (comment).

@rsc rsc self-assigned this Jan 12, 2023
@rsc
Copy link
Contributor

rsc commented Jan 12, 2023

One minor change from the accepted proposal: writing the text in the spec it turns out to be simplest to say that panic(nil) causes a run-time error, in which case the type should be runtime.PanicNilError, not runtime.PanicNil.

@DeedleFake
Copy link

For the specific case of panic(nil), the evidence is that there is a lot of code out there subtly broken today, which this change will fix, and there is hardly any code out there depending on panic(nil) showing up as a recover of nil, which this change will break. So the balance came out in favor of a global fix.

This is a good point. I still don't like the change being global, but it is definitely true that there is almost definitely more code out there failing to handle a recover from a panic(nil) properly than code that depends on it. I don't think there's a lot of code producing nil panics, though. It can only happen if you do purposefully call panic(nil) or pass it a nil interface. It's true that code that uses it might be relying on the recover() behavior of other modules that are then updated to use the new behavior, thus breaking the code creating the nil panics, but that's the whole point of module versions. If module A depends on a specific version of module B, then that version of module B will continue working as module A expects because it's literally the same code. Making the change global will break that version connection, effectively making it pointless.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/461956 mentions this issue: runtime: replace panic(nil) with panic(new(runtime.PanicNilError))

@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jan 19, 2023
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 19, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/462293 mentions this issue: cmd/go: update test for change in panic(nil) behavior

gopherbot pushed a commit that referenced this issue Jan 20, 2023
panic(nil) now panics with runtime.PanicNilError.

For #25448

Change-Id: I58994aa80d4d11f0c5fcd988714f7b4b45c6b5ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/462293
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
@liggitt
Copy link
Contributor

liggitt commented Feb 13, 2023

As a point of reference, libraries which already explicitly handled nil panics may need to make changes like go-check/check#136 to treat a nil panic prior to go1.21 and a *runtime.NilPanicError result in go1.21+ as equivalent.

@rsc
Copy link
Contributor

rsc commented Mar 31, 2023

I don't think we explicitly decided the question of whether to change net/http to continue to accept panic(nil) as an alias for panic(http.ErrAbortHandler) in Go 1.21 programs (that is, programs that set go 1.21 in their go.mod file).

That is, I don't think we decided whether to include a change like the one in this file from the draft CL:
https://go-review.googlesource.com/c/go/+/452555/1/src/net/http/server.go

Given that we have the GODEBUG and that http.ErrAbortHandler has existed since Go 1.8, I am inclined to leave that change out. The only impact will be that panic(nil) will cause new logging on servers running in Go 1.21 mode.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/517036 mentions this issue: _content/doc/go1.21.html: add note about panic nil behavior

gopherbot pushed a commit to golang/website that referenced this issue Aug 8, 2023
For golang/go#25448.

Change-Id: I775ad6d8d8952da8fa94c39c44a2420501b63b57
Reviewed-on: https://go-review.googlesource.com/c/website/+/517036
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
Long ago we decided that panic(nil) was too unlikely to bother
making a special case for purposes of recover. Unfortunately,
it has turned out not to be a special case. There are many examples
of code in the Go ecosystem where an author has written panic(nil)
because they want to panic and don't care about the panic value.

Using panic(nil) in this case has the unfortunate behavior of
making recover behave as though the goroutine isn't panicking.
As a result, code like:

	func f() {
		defer func() {
			if err := recover(); err != nil {
				log.Fatalf("panicked! %v", err)
			}
		}()
		call1()
		call2()
	}

looks like it guarantees that call2 has been run any time f returns,
but that turns out not to be strictly true. If call1 does panic(nil),
then f returns "successfully", having recovered the panic, but
without calling call2.

Instead you have to write something like:

	func f() {
		done := false
		defer func() {
			if err := recover(); !done {
				log.Fatalf("panicked! %v", err)
			}
		}()
		call1()
		call2()
		done = true
	}

which defeats nearly the whole point of recover. No one does this,
with the result that almost all uses of recover are subtly broken.

One specific broken use along these lines is in net/http, which
recovers from panics in handlers and sends back an HTTP error.
Users discovered in the early days of Go that panic(nil) was a
convenient way to jump out of a handler up to the serving loop
without sending back an HTTP error. This was a bug, not a feature.
Go 1.8 added panic(http.ErrAbortHandler) as a better way to access the feature.
Any lingering code that uses panic(nil) to abort an HTTP handler
without a failure message should be changed to use http.ErrAbortHandler.

Programs that need the old, unintended behavior from net/http
or other packages can set GODEBUG=panicnil=1 to stop the run-time error.

Uses of recover that want to detect panic(nil) in new programs
can check for recover returning a value of type *runtime.PanicNilError.

Because the new GODEBUG is used inside the runtime, we can't
import internal/godebug, so there is some new machinery to
cross-connect those in this CL, to allow a mutable GODEBUG setting.
That won't be necessary if we add any other mutable GODEBUG settings
in the future. The CL also corrects the handling of defaulted GODEBUG
values in the runtime, for golang#56986.

Fixes golang#25448.

Change-Id: I2b39c7e83e4f7aa308777dabf2edae54773e03f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/461956
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
Long ago we decided that panic(nil) was too unlikely to bother
making a special case for purposes of recover. Unfortunately,
it has turned out not to be a special case. There are many examples
of code in the Go ecosystem where an author has written panic(nil)
because they want to panic and don't care about the panic value.

Using panic(nil) in this case has the unfortunate behavior of
making recover behave as though the goroutine isn't panicking.
As a result, code like:

	func f() {
		defer func() {
			if err := recover(); err != nil {
				log.Fatalf("panicked! %v", err)
			}
		}()
		call1()
		call2()
	}

looks like it guarantees that call2 has been run any time f returns,
but that turns out not to be strictly true. If call1 does panic(nil),
then f returns "successfully", having recovered the panic, but
without calling call2.

Instead you have to write something like:

	func f() {
		done := false
		defer func() {
			if err := recover(); !done {
				log.Fatalf("panicked! %v", err)
			}
		}()
		call1()
		call2()
		done = true
	}

which defeats nearly the whole point of recover. No one does this,
with the result that almost all uses of recover are subtly broken.

One specific broken use along these lines is in net/http, which
recovers from panics in handlers and sends back an HTTP error.
Users discovered in the early days of Go that panic(nil) was a
convenient way to jump out of a handler up to the serving loop
without sending back an HTTP error. This was a bug, not a feature.
Go 1.8 added panic(http.ErrAbortHandler) as a better way to access the feature.
Any lingering code that uses panic(nil) to abort an HTTP handler
without a failure message should be changed to use http.ErrAbortHandler.

Programs that need the old, unintended behavior from net/http
or other packages can set GODEBUG=panicnil=1 to stop the run-time error.

Uses of recover that want to detect panic(nil) in new programs
can check for recover returning a value of type *runtime.PanicNilError.

Because the new GODEBUG is used inside the runtime, we can't
import internal/godebug, so there is some new machinery to
cross-connect those in this CL, to allow a mutable GODEBUG setting.
That won't be necessary if we add any other mutable GODEBUG settings
in the future. The CL also corrects the handling of defaulted GODEBUG
values in the runtime, for golang#56986.

Fixes golang#25448.

Change-Id: I2b39c7e83e4f7aa308777dabf2edae54773e03f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/461956
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
Long ago we decided that panic(nil) was too unlikely to bother
making a special case for purposes of recover. Unfortunately,
it has turned out not to be a special case. There are many examples
of code in the Go ecosystem where an author has written panic(nil)
because they want to panic and don't care about the panic value.

Using panic(nil) in this case has the unfortunate behavior of
making recover behave as though the goroutine isn't panicking.
As a result, code like:

	func f() {
		defer func() {
			if err := recover(); err != nil {
				log.Fatalf("panicked! %v", err)
			}
		}()
		call1()
		call2()
	}

looks like it guarantees that call2 has been run any time f returns,
but that turns out not to be strictly true. If call1 does panic(nil),
then f returns "successfully", having recovered the panic, but
without calling call2.

Instead you have to write something like:

	func f() {
		done := false
		defer func() {
			if err := recover(); !done {
				log.Fatalf("panicked! %v", err)
			}
		}()
		call1()
		call2()
		done = true
	}

which defeats nearly the whole point of recover. No one does this,
with the result that almost all uses of recover are subtly broken.

One specific broken use along these lines is in net/http, which
recovers from panics in handlers and sends back an HTTP error.
Users discovered in the early days of Go that panic(nil) was a
convenient way to jump out of a handler up to the serving loop
without sending back an HTTP error. This was a bug, not a feature.
Go 1.8 added panic(http.ErrAbortHandler) as a better way to access the feature.
Any lingering code that uses panic(nil) to abort an HTTP handler
without a failure message should be changed to use http.ErrAbortHandler.

Programs that need the old, unintended behavior from net/http
or other packages can set GODEBUG=panicnil=1 to stop the run-time error.

Uses of recover that want to detect panic(nil) in new programs
can check for recover returning a value of type *runtime.PanicNilError.

Because the new GODEBUG is used inside the runtime, we can't
import internal/godebug, so there is some new machinery to
cross-connect those in this CL, to allow a mutable GODEBUG setting.
That won't be necessary if we add any other mutable GODEBUG settings
in the future. The CL also corrects the handling of defaulted GODEBUG
values in the runtime, for golang#56986.

Fixes golang#25448.

Change-Id: I2b39c7e83e4f7aa308777dabf2edae54773e03f5
Reviewed-on: https://go-review.googlesource.com/c/go/+/461956
Reviewed-by: Robert Griesemer <gri@google.com>
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Russ Cox <rsc@golang.org>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/536955 mentions this issue: runtime: document GODEBU panicnil values

gopherbot pushed a commit that referenced this issue Jan 17, 2024
Updates #25448

Change-Id: Ia1b7a376f5175f67e14ad4bd065d6e8ad5250d38
Reviewed-on: https://go-review.googlesource.com/c/go/+/536955
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Keith Randall <khr@golang.org>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Updates golang#25448

Change-Id: Ia1b7a376f5175f67e14ad4bd065d6e8ad5250d38
Reviewed-on: https://go-review.googlesource.com/c/go/+/536955
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Keith Randall <khr@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LanguageChange NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests