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

Jump to bottom

spec: panicking corner-case semantics #10458

Open
mdempsky opened this issue Apr 14, 2015 · 4 comments
Open

spec: panicking corner-case semantics #10458

mdempsky opened this issue Apr 14, 2015 · 4 comments
Milestone
Unplanned

Comments

@mdempsky
Copy link
Member

A few corner-cases regarding panicking that I noticed in the Go spec:

1) What happens if a panic occurs while a goroutine is already panicking?

Gc/gccgo seem to allow recursive panicking and recovering: http://play.golang.org/p/tBkwgyzmuT

But if a deferred function panics without any recovery, then the original panic is lost: http://play.golang.org/p/KqwGiWGMAx

2) The Go spec says "The return value of recover is nil if any of the following conditions holds: [...] recover was not called directly by a deferred function."

Gc/gccgo though seem to also have recover return nil in deferred functions if they were executed because of normal (i.e., non-panicking) function return: http://play.golang.org/p/a-fl_9Gga0

3) The Go spec says "Suppose a function G defers a function D that calls recover and a panic occurs in a function on the same goroutine in which G is executing. [...] If D returns normally, without starting a new panic, the panicking sequence stops."

Nit: I think the intention here is understood, but the wording could probably be improved somewhat. As quoted in item 2 above, the spec later explicitly refers to deferred functions that "directly" call recover. Omitting "directly" here suggests that indirect calls to recover should still stop the panicking sequence, but they'll return nil instead of the panic value.
@mdempsky mdempsky assigned mdempsky and griesemer and unassigned mdempsky Apr 14, 2015
@ianlancetaylor
Copy link
Contributor

Unfortunately I don't understand what you are suggesting. To me the spec seems accurate and complete.

You seem to suggest that we should omit "directly," but that would be a clear change in behaviour. Indirect calls to recover do not stop the panicking sequence. They simply return nil and the panic continues.

@mdempsky
Copy link
Member Author

@ianlancetaylor Are you referring to just item 3, or you think the spec is accurate and complete for the first two items I listed too?

Rephrasing my concern about the wording in item 3, currently the spec seems to admit this behavior for recover:

func recover() interface{} {
    // 1. Apply the "stop panicking" logic from the "The recover function allows a program [...]" paragraph.
    if ("called by function D [possibly indirectly?], which was deferred by panicking function G") {
        stopPanicking()
    }

    // 2. Apply the "return nil" logic from the next paragraph.
    if ("goroutine is not panicking" || "not *directly* called by a deferred function") {
        return nil
    }

    // 3. Continue with the previous paragraph and return the value from panicking.
    return valueFromPanicking()
}

The nit I see is the stop-panicking logic and the what-value-is-returned logic are separately described with slightly different wording.

I think by reorganizing the section a bit, it could be clearer that the correct behavior is something like:

func recover() interface{} {
    // 1. This case always return nil without doing anything.
    // (Note: No need to test for "not panicking", because that's just a special case.)
    if ("not *directly* called by a function D, deferred by a panicking function G") {
        return nil
    }

    // 2. Stop panicking and return the panic value.
    stopPanicking()
    return valueFromPanicking()
}

@ianlancetaylor
Copy link
Contributor

Thanks, I think I see what you mean now. We should clarify that we only stop panicking for the cases where recover returns non-nil.

@ianlancetaylor ianlancetaylor added this to the Go1.5Maybe milestone Jun 3, 2015
@rsc rsc modified the milestones: Unplanned, Go1.5Maybe Jul 15, 2015
@gopherbot
Copy link

Change https://golang.org/cl/189377 mentions this issue: spec: specify the behavior of recover and recursive panics in more detail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@mdempsky @rsc @ianlancetaylor @griesemer @gopherbot