I have just noticed that that Concurrent.Promise()->create() just plainly calls the passed executor with success and failure as callbacks/arguments.
It is my understanding that the whole purpose of the executor is so that any errors thrown (intentionally or inadvertently) can be caught and converted to a rejection of the promise. If not for that (which is handy), I cannot see any purpose or justification for the executor.
I think this qualifies as a bug that should just be fixed. I have pushed a commit to that extent to master. I think this should also go onto 8.0. Objections? Agreement all around?
However, this opens up another issue, if rejecting with the error caught results in double finalisation, the error thrown about that (i.e. out of Promise()->create() from the callers perspective) will only have very limited hints about the "original" error thrown (it is present in the backtrace as argument to [-2], e.g. failure(), as the original error object/array, but visibility in prints is severly limited).
I wonder if doing something along the lines of
diff --git a/lib/modules/Concurrent.pmod b/lib/modules/Concurrent.pmod index dbbd509334..01e1d19e98 100644 --- a/lib/modules/Concurrent.pmod +++ b/lib/modules/Concurrent.pmod @@ -776,15 +776,27 @@ class Promise if (executor) { mixed err = catch(executor(success, failure, @extra));
- // This unfortunately does hide the real error in case of - // double-finalization, i.e. - // fail("I reject!"); - // error("I rejected.\n"); - // in the executor will leave our caller with an error about the - // Promise already having been finalized, not giving anyone too much - // information about where the double-finalization occured... - if (err) - failure(err); + if (err) { + // If rejecting the promise fails due to double-finalization, we + // outright lie about the backtrace here so that it includes the + // position where the "original" error is double thrown, which + // probably makes most related debugging easier but maybe some very, + // very hard... + if (mixed err2 = catch(failure(err))) { + mixed bt; + + catch(bt = err[1]); + + catch { + if (arrayp(err2) && arrayp(bt) && sizeof(bt) + && Program.inherits(bt[-1], __builtin.backtrace_frame)) { + err2[1] = err[1] + err2[1][<2..]; + } + }; + + throw(err2); + } + } } }
TL;DR mangling the backtrace to pretend control flow was: site of error() -> Promise()->create() -> failure() -> finalise()
would do more good than harm. I cannot say, on one hand I think this would really help people running into this problem, but at the same time it admittedly is just all kinds of wrong.