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.