Closed
Description
Go version
devel go1.23-a3a584e4ab Wed May 29 13:52:34 2024 +0000
Output of go env
in your module/workspace:
n/a since this was tested on the playground (gotip)
What did you do?
Run this on the playground
package main
import (
"fmt"
"iter"
"runtime"
)
func main() {
fmt.Println(runtime.Version())
defer func() {
if err := recover(); err != nil {
fmt.Println(err)
}
}()
next, _ := iter.Pull[int](func(yield func(int) bool) {
yield(1)
panic("!")
})
fmt.Println(next())
fmt.Println(next())
}
https://go.dev/play/p/_BjecTfmPE8?v=gotip
What did you see happen?
The panic is not caught at recover()
devel go1.23-a3a584e4ab Wed May 29 13:52:34 2024 +0000
1 true
panic: !
goroutine 4 [running]:
main.main.func2(0x0?)
/tmp/sandbox4071758638/prog.go:18 +0x2e
iter.Pull[...].func1()
/usr/local/go-faketime/src/iter/iter.go:75 +0x10b
created by iter.Pull[...] in goroutine 1
/usr/local/go-faketime/src/iter/iter.go:59 +0x119
What did you expect to see?
The panic is caught at recover()
Metadata
Metadata
Assignees
Type
Projects
Status
Done
Status
Approved
Relationships
Development
No branches or pull requests
Activity
hajimehoshi commentedon May 30, 2024
cc @golang/compiler
mknyszek commentedon May 30, 2024
iter.Pull
isn't explicitly defined as running on another goroutine, but in practice that is what it's doing, leading to the behavior you see here.We've gone back and forth on whether it should be documented in terms of goroutine+channel semantics (that is,
iter.Pull
is defined as having the semantics of running the iterator on another goroutine, withnext
andstop
communicating with it via channel), but decided to leave it more ambiguous for now since it accepts more possible implementations, including the current one (though goroutine+channel semantics are a valid implementation ofiter.Pull
).I think maybe what does have to be documented is that panics do not propagate through an
iter.Pull
iterator. I think this must be true of any implementation (and AFAICT, it is true of all the implementations we've thought of so far), because the abstraction here is something like a coroutine, and it's not clear to me that panics should unwind through coroutine boundaries.CC @dr2chase @aclements
hajimehoshi commentedon May 30, 2024
IIUC, the actual implementation is a coroutine rather than a regular goroutine. If the semantics is the same as goroutine, I was wondering why a coroutine is used instead of a regular goroutine. Is this for performance? If so, now we have issues around Cgo with iter.Pull (#67499), isn't this a kind of early optimization? Or, might the semantics be changed in the future?
mknyszek commentedon May 30, 2024
It's really a fast, direct switch between goroutines that doesn't go through the scheduler. This is functionally similar to a coroutine switch, hence why it's called
coroswitch
in the implementation.The semantics are explicitly not the same as a goroutine. The semantics are a little less well-defined than that at the moment.
Also, even if we formalized the notion of coroutines in the language, I'm not sure I agree that a panic should be able to cross coroutine boundaries.
Yes, it's for performance. The direct switch is very, very cheap compared to implementing the same thing with channels. I get what you're saying about the optimization possibly happening too early (this was discussed at length), but the performance of
iter.Pull
was considered too important. The issues around cgo will hopefully not be permanent, but they're also a big part of why the semantics are not defined as goroutines.jimmyfrasche commentedon May 30, 2024
Are there many languages where an exception in a coroutine isn't propagated?
hajimehoshi commentedon May 30, 2024
An exception in a coroutine is caught in Ruby and JavaScript:
Ruby
https://try.ruby-lang.org/playground/#code=def+foo%0A++Fiber.new+do%0A++++raise+'error!'%0A++end%0Aend%0A%0Abegin%0A++f+%3D+foo()%0A++f.resume%0Arescue+%3D%3E+e%0A++puts+e%0Aend%0A&engine=cruby-3.2.0
JavaScript
https://jsfiddle.net/kru9b5tg/
mknyszek commentedon May 31, 2024
Oh, that's interesting... At least in JS it really makes sense that exceptions would propagate through. On the caller side (and the definition side!) it just looks almost like calling a regular function, especially with the syntax.
I think what's tricky about Go is that it's already possible for functions to do more complicated things with a closure (like sending it to another goroutine) inside of a function call, and this is a somewhat normal thing to do.
That being said, I do think you changed my mind about:
I think there's a good argument to be made about that.
However, for
iter.Pull
specifically, it was a soft goal of ours that a possible implementation ofiter.Pull
would be using just plain goroutines and channels as part of the semantics. That goal does seem to require panics not to propagate.jimmyfrasche commentedon May 31, 2024
A goroutine implementation could capture the panic and send it to the other side to be re-panic'd when next/close get invoked. https://go.dev/play/p/ltXl_x_CPvm
mknyszek commentedon May 31, 2024
@jimmyfrasche That's true, that you can re-panic, but what I meant by propagation is that the original panic stack follows (for example, if the panic is uncaught, you lose the context by re-panicking). But maybe that doesn't matter? The JS example at least doesn't seem to provide any context on an uncaught exception (though that alone isn't a reason to replicate the behavior).
I think I'm personally still leaning toward the panic not propagating just because that seems more consistent within the language. Is there any existing API that runs a function on another goroutine and possibly re-panics like this? Coroutines don't currently exist in the language, so it would be a little weird to say
iter.Pull
behaves like coroutines in other languages.mknyszek commentedon May 31, 2024
After discussing offline with @aclements, I think I'm convinced that we should propagate panics. We can start out by just re-panicking: the user-visible behavior is the most important part for this release cycle.
Here's the information that changed how I was leaning.
Although the primary example that came to mind (
exec.Command
) doesn't propagate panics from the goroutines it starts, there are a few examples of where, going forward or already today, we do propagate panics.sync.OnceFunc
explicitly propagates panics from its callback, and does so even if thesync.OnceFunc
is accessed multiple times (it won't surprisingly succeed on future calls). Although it doesn't create a goroutine, it is an API that takes a function an executes it on behalf of the caller.Wait
.Furthermore, we wanted to leave the door open to a compiler-driven CPS transform as a valid implementation of an
iter.Pull
call. That implementation actually does suggest propagating panics.And this is a situation of "if we can, we should," and we definitely "can," since the implementation controls the creation of the
iter.Pull
. While std is currently a bit inconsistent about forwarding panics, my general sense is that the project wants to move toward propagating them.I will send a CL later to forward panics.
[-]iter: panic in `iter.Pull` is not recovered[/-][+]proposal: iter: `iter.Pull` should forward panics[/+]33 remaining items