RFC: Make panic from unpark() illegal?

[nikomatsakis]

I recently extended Rayon to support futures (https://github.com/nikomatsakis/rayon/pull/193). In doing so, I came across one import question about the Unpark trait. When you spawn a future F in Rayon, we create a new future F' representing the result of that computation. If someone polls F', then they may record an Unpark implementation in F'. When F completes, and F' is signaled, we will invoke unpark() to let people know that F' is ready to be re-polled. The question is: what do we do if unpark() should panic? There doesn't really seem to be a good answer.

The problem is that unpark() here is being called from an unrelated thread -- effectively "somewhere in the Rayon runtime". It doesn't make sense for the panic to propagate here really. Rayon generally tries to capture panics and channel them somewhere appropriate -- so e.g. if you spawn a task in scope S, and that task panics, then the panic will be propagated to the function call that created the scope S. But in this case there is nowhere for us to propagate the panic to: the future F' is already complete -- that's why we were able to call unpark() -- so we can't set it to an error or anything.

I am not sure what Rayon should do in the event of a panic -- either it would be swallowed (perhaps dumped to stderr?) or else we abort the process? Maybe we will make the behavior configurable, actually, with a customizable "unhandled panic handler" or something.

In any case, it seems like this will be a common problem for runtimes. At minimum, I propose that there should be a comment that unpark() methods are not permitted to panic and that the results of doing so are "unspecified" in some sense (presumably not including actual UB, since it is not an unsafe fn and it seems annoying to make it one). This would give Rayon more license to abort. =)

This does have some implications. In particular, an unpark() impl should not go ahead and immediately do work "in situ" -- at least not without capturing panics and making some provision to channel those panics elsewhere.

Thoughts?

[nikomatsakis]

Hmm, after some thought I wonder if I have overstated the case. In Rayon, at least, futures are spawned inside a scope S. We could propagate the panic to the scope S itself (panics w/in the future fn are normally transmitted to the one that polls the future).

[Diggsey]

Wouldn't there still be exceptional cases (eg. OOM) where panicking may be the only reasonable thing for unpark() to do?

[nikomatsakis]

@Diggsey that's not clear to me. It's possible that in such cases it'd be better to find another way to signal the problem. But I think I'm resolved now that I can capture and pipe unpark() panics to somewhere reasonable, so maybe I don't care.

[nikomatsakis]

@Diggsey in other words, probably yes :)

[alexcrichton]

This sounds like a good idea to me. I agree that this would largely just be a documentation level concern as actually enforcing this statically seems too egregious ergonomically.

The only case I'd be worried about, which you mentioned, is the case where unpark decides to go an poll the future that it's managing. This was somewhat common in the past but nowadays it's pretty rare, so we may be able to get by with just saying that it's a "weird architecture"

[nikomatsakis]

So, it occurred to me that, at least in Rayon, I don't expect to always have a place where I can propagate a panic. Therefore, I propose the following:

The documentation should say something like:

"Since unpark() may be invoked from arbitrary contexts, it should endeavor not to panic and to do as little work as possible. However, it is not guaranteed not to panic, and callers should be wary. If a panic occurs, that panic may or may not be propagated to the end-user."

[nikomatsakis]

I'm not trying to draw too much from Rayon; it seems to me that any executor will have a similar problem (i.e., futures-cpupool).

[alexcrichton]

@nikomatsakis that sounds like an excellent clause to me!