Increases promise alignment with spec

[d3nd3]

#2450
#2227

This code aims to make promise behave closer to ES6 specs.

[d3nd3]

Let me know your thoughts on this, once you have some free time. I did spend quite a lot of effort arranging this, happy to have it on the sidelines in case needed though.

[gfwilliams]

Thanks - sorry for the delay, I was away last week and I'm still catching up a bit - I wasn't sure if you thought this was quite ready for review.

At first glances it seems hard to narrow down exactly what's changed because it looks like everything has :) I need to take some time and build this and test it out.

Does this run through the existing local tests (bin/espruino --test-all) and pass them still? I know you did this to fix some issues with the existing implementation - is it possible to get them added to the tests that are there so we can make sure that they don't regress again in future if something changes?

[gfwilliams]

Just looked into this and ran --test-all myself to check, but it looks like this breaks tests/test_promise3.js and tests/test_promise4.js, both of which work in Node.js. Was that expected?

And as above, it'd be really nice to have tests added for the issues that you fixed to ensure they don't end up broken in the future.

[d3nd3]

When you ran them in node.js , what was the output?

For me, I get Unhandled promise rejection, which is correct and due to the line:

p.then(function(value) { // 1
}
);

calling p.then() again on the initial promise, sets up a new chain, and because the second chain doesn't have a catch handler, it has to error. Even though the first chain does have a catch handler.

test_promise4.js has the same idea in it, D1 is correct, but since it errors, the 'result' variable is not set, so the test fails.

Those two tests should be fixed for that condition.

[gfwilliams]

Thanks! I'd previously run them in Node by copy/pasting them into the REPL so I could check the value of result, and I'd obviously missed the exception that appeared at that point (and the end result was as expected if it kept executing after the exception). As you say running them as files in node breaks. I've just updated the tests and added 3 new ones for the outstanding issues with promises.

However now I see fails on:

• test_promise4.js - reports r1Cundefined and not r1D1 (appears to be executing then even when it's been rejected)
• test_promise15.js - we get Resolving a Promise with a value that is a Promise is not currently supported - this is what happens in Promises: resolve a new Promise #2450 which I think you might have thought was fixed? It's possible this was because of a merge issue though as that change went in on 2024-01-15?
• test_promise17.js - returns a twice when it should work as in Promise implementation isn't correct #2227

So it seems this still might not fix the original reported 2 issues fully?

[d3nd3]

Yes it must be because of the merge issue because I tested them here, and they pass (whilst using my fork 'promises-rework')

[gfwilliams]

Well I have no idea what happened but I've been unable to merge this without introducing errors interpreting promises - so I just took the file directly from this PR and overwrote what's there before. But this is now merged, and yes, the tests do all appear to pass.

Thanks for the PR - but it would have been nice if it included the tests that it was attempting to fix.

[d3nd3]

Yes, sorry about that, I didn't have the time and when I did, I had forgot most of the details of the implementation.

I want to just say that the crux of the issue with the previous implementation was that, the 'chain' pointer (linked list) was used for resolve/reject pass-through when there is no 'callback' defined. Yet as pointed out in #2227 , this will not work because one promise can have multiple 'children' (when calling .then on it). This means one promise can only ever have one chain. You tried to get around it by removing the chain from the promise after its resolved, but that still means one chain for unresolved promises.

As for #2450 , its similar issue, the chain needs to to be an array chain, instead of linear chain. But making the constructor(executor) be able to return a promise is similar to your code that enables .then callback to also return a promise, which isn't so difficult.

I implemented it like :

So now there can be multiple callbacks and chain for each Promise. I hope that helps understand the changes. I suppose your initial premise about promises was lacking, thus why the code was how it was, but for approximating to how you thought promise's work I really liked how you arranged it originally.

[mariusGundersen]

Thank you so much for fixing this!

[gfwilliams]

Reopening because of #2495 - I've reverted this for now as at least it's just a single file change, and I don't want to have literally every use of Promises in the Espruino APIs broken

[d3nd3]

sad thing is, I can't remember why I am using a container object, I need time to remember. Its possible we don't need it?

[d3nd3]

I have remembered the reasoning for using the container object. Its because:
https://github.com/d3nd3/Espruino/blob/8859819a0fe1305c8567e79ef2eb3cb82622ae5c/src/jswrap_promise.c#L187

When resolving a promise, it needs a unique instance of "resolved" , that is why resolved sits inside the prombox next to the promise. So think of it as spawning a new instance of 'resolved', which is needed for the recursion, yet re-using the promise.

The prombox is bound as 'this' to the resolve,reject callbacks in promise constructor.

// 'this' == prombox.
    if (resolve) jsvObjectSetChild(resolve, JSPARSE_FUNCTION_THIS_NAME, promBox);
    if (reject) jsvObjectSetChild(reject, JSPARSE_FUNCTION_THIS_NAME, promBox);

So when javascript land calls resolve or reject via constructor executor, the prombox is passed via first 'this' argument, that is how the container doesn't need its reference stored. This is making me wonder if the only time that the prombox is required, is if you need to resolve it in C. If the C function is returning a promise, it is planning to resolve it internally, thus internally the prombox pointer would be needed. If the user creates the promise with new Promise((resolveFunc,rejectFunc)=>{//executor func}), the resolveFunc and rejectFunc have their this bound to the promBox as above.

Instead of too many lines in all api instances that return a promise, we could create another promise function that extracts the promise from the prombox to keep line count shorter? But the entire reasoning behind returning a promise in C, is to resolve it in C too, which requires the promBox pointer and calls the jspromise_resolve function which is the same function which is passed as arguments to the executor in the Promise constructor.

If the promise is created in javascriptland the scripter has control of the resolve.

[d3nd3]

// otherwise the returned promise will be fulfilled with the value.
  JsVar *promBox = jspromise_create();
  if (!promBox) return 0;
  promise = jsvObjectGetChildIfExists(promBox, JS_PROMISE_PROM_NAME);
  if (promise) {
    jspromise_resolve(promBox, data);
  }
  jsvUnLock(promBox);
  return promise;

In the jswrap_promise_resolve function I had already handled this type of problem, so I guess this is one solution. But I don't really want to copy paste that everywhere, so having another function seems nicer.

JsVar *jspromise_extract(JsVar * promBox) {
    JsVar * promise = jsvObjectGetChildIfExists(promBox, JS_PROMISE_PROM_NAME);
    return promise;
}

Up to you, mb the function jspromise_create() should be renamed to jspromise_create_box()

[mariusGundersen]

So when javascript land calls resolve or reject via constructor executor, the prombox is passed via first 'this' argument, that is how the container doesn't need its reference stored.

Why can't you use the promise directly? Why is the wrapping box needed?

[mariusGundersen]

Ok, it's to handle the scenario where you resolve a promise with another promise:

new Promise((resolveOuter) => {
  resolveOuter(
    new Promise((resolveInner) => {
      setTimeout(resolveInner, 1000);
    }),
  );
});

I'm this case the outer promise will be resolved but not yet fulfilled (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#description).

[gfwilliams]

If there's any way around it I'd really rather not have two separate types of promise object kicking around external to jswrap_promise.c (if it's used inside, fine) - it feels like it's going to cause all kinds of headaches in the future where in code someone creates the wrong promise object inside the Espruino codebase, or thinks that they can call promise_resolve on something passed from JS land, or pass the promise from promise_create into JS land. At the very least I guess we should rename promise_X to promBox_X in all the code and rename blePromise/etc to blePromBox (but that's quite a lot of changes).

I'm not quite sure I understood from the diagram you drew, but looking at the code the Box stores a single value for the resolved state, and a link to the Promise? But the link to the promise is never updated once set, so every box is linked to just one promise... and you have multiple boxes, one for each time a promise is resolved?

But it feels like the next item in the promise 'tree' can't even be executed until the first one has resolved, so presumably only one 'box' is active at a time... And when you do .then you get a new promise object created anyway, which would have its own box too?

Is there a reason we can't have the 'current' Box linked from the promise itself? So then we only pass the promise around, but when we need to set resolved/etc we can just follow the link to Box and set that?

I'm still not sure I get it - Do you think you could give me an example of when it wouldn't work for some code in Espruino to use promise_create and promise_resolve on the actual promise rather than the box?

I know there wasn't much documentation before (although there were a few in-code comments), but it would be really nice to have some kind of code documentation at the end of this, even if it's just what you said above at the top of the file explaining what's going on with promBox. Right now there are even less comments than there were before.

I've spent the last hour trying to dig through this and get my head around it, and I'm still not there - and when the original author says "I had forgot most of the details of the implementation" it's worries me about how maintainable this is going to be in the future.

[d3nd3]

The code is heavily inspired from : https://github.com/humanwhocodes/pledge
and the tutorials he provides:
https://humanwhocodes.com/blog/2020/09/creating-javascript-promise-from-scratch-constructor/
https://humanwhocodes.com/blog/2020/09/creating-javascript-promise-from-scratch-resolving-to-a-promise/
https://humanwhocodes.com/blog/2020/10/creating-javascript-promise-from-scratch-then-catch-finally/
https://humanwhocodes.com/blog/2020/10/creating-javascript-promise-from-scratch-promise-resolve-reject/

also:
https://www.ecma-international.org/ecma-262/11.0/index.html#sec-promise-objects

Its not like i am completely reinventing the wheel here, understanding the above helps understand my code too. The reason for lack of comments is because I was under the impression you preferred less comments. I could adjust it so that it doesn't use a box, and has more comments, it seems possible.

[gfwilliams]

I'm just looking at some of the promise test cases and how they work, and I'm wondering whether everything is easier than I expected...

In the original code I was trying to have an array as a chain of promises, because I hadn't realised that .then should return a brand new promise each time - so everything ended up being a bit of a hack.

So...

• Promises would have a state, value (if resolved/rejected) and a list of 'children'
• new Promise() creates a promise,
• promise.then/catch create a new promise and add it to the 'children' list of the first promise (if the first promise is already resolved, it queues a resolve immediately)
• Now when something resolves/rejects, it just goes through all its children and queues for them to be executed.
• If something resolves returning a promise as data, we just move the children of the current promise onto that new promise and we're good (I think?)

Does that sound right, or am I missing something obvious? Is there a need for the boxes in here?

[d3nd3]

For my code to not use box, I would make the is_resolved field and promise to be children of the 'resolve' function object and reject function object instead, as the spec does, I think that could work?

[gfwilliams]

Ok, looks like I posted this just as you posted your response!

Thanks - those links look really helpful. I'm not entirely sure I understand what you mean about making them fields of the function objects, but if you think it could work that would be perfect. If it matches what happens in the spec more then that's great.

I was under the impression you preferred less comments

Oh no, I like comments! Not:

function getFoobar() // this gets the foobar

But actually explaining what's going on is very helpful - especially here where the jswrap_promise has been a bit of a minefield in the past :)

[d3nd3]

So given a promise p1.

It can have many reactions attached to it via many separate .then calls. Chained or unchained.

let pX be array of reactions

When p1 resolves, all pX callbacks/promises _directly_ attached to it (lets call these reactions, which are a promise and a callback(can be rejectCB or fullfillCB)) are fired, the return value of the fired CB in turn will resolve pX. pX is the promise that is returned from p1.then(). The CB function is the function passed to .then, :
p1.then(fullfillCB(){},rejectCB(){})

You do not have to iterate the chain, that will happen automatically when each resolve like a domino effect. If a callback function returns another promise, then the outer promise will not resolve until the inner promise resolves, and that can be recursive endlessly too.

Your points and descriptions above sound interesting and solid, I never thought about the idea of moving the children, could work. If you wanna try implement yourself a neat solution, i'm down.

But I must say that the promise concept is a lot to digest, that is why its so difficult to implement. If we manage to keep our heads afloat we should be alright.

If something resolves returning a promise as data, we just move the children of the current promise onto that new promise and we're good (I think?)

The current technique is to call .then on the innner promise, with resolve as the fullfillCB argument, so that when inner promise resolves, it will resolve the outer promise too.
This is making the outer promise a child of the inner promise.

Your idea is move all children which can also be achieved by moving the parent of all children (which is the outer promise) which would indirectly trigger all children anyway, if you know what I mean? So yea, we kinda on the same page anyway.

[d3nd3]

Okay, so I have remembered why I used a promBox instead of doing as the spec does by making the prom and is_resolved children of the resolve function. The reasoning is because the resolve function doesn't always exist in javascript land as a function. It is sometimes a promise created in C land, where there is no corresonding JsVar resolve function, just the _jswrap_promise_resolve and _jswrap_promise_reject functions, but when the promise constructor is called in javascript, the functions are converted into JsVars:

JsVar *jsResolve = jsvNewNativeFunction((void (*)(void))_jswrap_promise_resolve, JSWAT_VOID|JSWAT_THIS_ARG|(JSWAT_JSVAR<<JSWAT_BITS));
    JsVar *jsReject = jsvNewNativeFunction((void (*)(void))_jswrap_promise_reject, JSWAT_VOID|JSWAT_THIS_ARG|(JSWAT_JSVAR<<JSWAT_BITS));

So I didn't want to create two native functions just for the sake of the c land manual resolve cases, and I wouldn't know where to store them. So because I lacked imagination on memory management in c land, that is when I came up with the idea of the prom box.

[d3nd3]

https://humanwhocodes.com/blog/2020/09/creating-javascript-promise-from-scratch-constructor/#creating-the-resolving-functions

This part in particular is what i'm referring to.

So I guess I do need a function that creates the resolving functions and returns them as JsVars, then you could resolve a promise in c land by grabbing the resolve functions.

[d3nd3]

https://esdiscuss.org/topic/es6-promises-alreadyresolved-needed

This seems interesting, so the part we don't fully understand is why it needs a separate isResolved, but mb this sheds some light? I think we don't support this 'locking in' feature anyway, so mb its not needed.

https://github.com/domenic/promises-unwrapping/blob/master/README.md

One guys suggests:
NB: A promise gets locked-in when it is resolved with another promise. As that second promise may be pending, the first promise is thus resolved, but still pending.

So I'm guessing its ye to prevent multiple resolve, for the case where the promise resolves another promise? Hm.

Ah so I think it means this :
The outer promise is resolved when it returns a promise, but it needs to be resolved a second time by the inner promise, even though its resolved. And so to do this they duplicate the resolved variable. Yes! Eureka!

[d3nd3]

This resolve call (at bottom right), when the inner promise resolves, has to succeed, even though the outer promise has its is_resolved field already set.

[gfwilliams]

Wow, thanks for digging into this - that's great!

So in the 'outer promise' case, you can call resolve with a promise, but the outer promise will still show as pending until that inner promise resolves, even though you can't resolve it a second time because of alreadyResolved.

My initial thought is that having a 4th state on the promise JS_PROMISE_STATE_PENDING_INNER_PROMISE would fix this, but I think it'd then fail if a promise returned a promise which returned a promise?

So I guess we're stuck with the box, or what I'd mentioned:

If something resolves returning a promise as data, we just move the children of the current promise onto that new promise and we're good (I think?)

But I feel like what you've done being in line with the spec makes a lot more sense.

So do you think it's possible to make promise_create just return a promise, and jspromise_resolve/reject to take a normal promise as before? It's possible they can just use a fake prombox with alreadyResolved=false as we know they'll only be called from Espruino where we won't double-resolve anyway.

All the rest of the your code can be basically the same

[d3nd3]

If something resolves returning a promise as data, we just move the children of the current promise onto that new promise and we're good (I think?)

I don't think this is that unique from what is already done. You still would have to call resolve eventually on the outer promise, which already was resolved. EDIT: Ah mb you right that because you not resolving the outer promise, but its children instead then you are not duplicating/resolving twice. I gotcha, I see now. But ye its state would still need to be updated. because of below regarding .then usage.

Resolved field is to prevent firing of resolve function twice ( prevent triggering execution of the children callbacks multiple times ). Imagine in a constructor

new Promise((resolve,reject)=>{
  resolve();
  resolve();
  return;
})

State field is used primarily in the jswrap_promise_then function to check if the promise has already fullfilled or rejected, in which case the callback should instantly be queued because there is no other event that would do it, if it already occured.

As for your other question, I am still thinking on it, but highly probable.

So do you think it's possible to make promise_create just return a promise, and jspromise_resolve/reject to take a normal promise as before? It's possible they can just use a fake prombox with alreadyResolved=false as we know they'll only be called from Espruino where we won't double-resolve anyway.

I see what you mean. The wrapper functions are only called from within espruino, correct. This sounds do-able.

[d3nd3]

Minor changes, what do you think?

Its now mb mergable since I updated to the master again. Why does the puckjs not compile?

[gfwilliams]

Thanks! If those changes work, that looks like it'd solve the problems? Looks great.

Looking at where jspromise_create_prombox is used I guess a minor optimisation might be to give it a JsVar **promise argument where it would return the new promise it created, so you don't have to immediately search in the returned object for it?

[d3nd3]

Looking at where jspromise_create_prombox is used I guess a minor optimisation might be to give it a JsVar **promise argument where it would return the new promise it created, so you don't have to immediately search in the returned object for it?

Thats a great idea!

[d3nd3]

XD, pushing slightly too often, I need to be more patient. But ye compile errors fixed. Locks currently not good, gotta find the lock problem. Hm.

[d3nd3]

I don't know how to find which variable/line of code is caused the LOCK assert, any ideas?
ASSERT(jsvGetLocks(var)>0) FAILED AT src/jsvar.c:847 not very helpful.

In the past I think I spammed trace and consoleprint lines everywhere.

My assert issue is coming from :
jsvUnLock(jspEvaluate(buffer, false)); in run_test() in main.c

Somehow there is a value returned from the evaluation of a script? What would that value be? How is it detecting the return value of my jswrap_promise_then() function, very confusing, one call to jsvUnLock() yet it targets my return value of jswrap_promise_then().

Actually its just at the end of each line I guess? How is the jspEvaluate calling jsvUnLock line by line? It doesnt' seem that way in the code, it seems as if its one large buffer. Kinda confused.

Is the rule about having Lock Count of >0 when returning variables true even for functions which are jswrap functions called from javascript land? I would assume not, I would assume they would have a lock count of 0 and stay alive via refs only? and if they had lockcount above 0, how would their lock count ever get to 0?

Nevermind, I have finally narrowed it down to this unlock:

NO_INLINE JsVar *jspParse() {
  JsVar *v = 0;
  while (!JSP_SHOULDNT_PARSE && lex->tk != LEX_EOF) {
    jsvUnLock(v);

I put :

if (jsvGetLocks(var)==0) raise(SIGTRAP);

in jsvUnLockInline

So this answers my question above, every line that is parsed is unlocked, so everything returned to javascript does need to be locked atleast once because its unlocked as its parsed.

[d3nd3]

Gotta fix a lock or two, then should be ready.

[d3nd3]

tests/manual/bangle2_fill.js
tests/test_262_FAIL.js
tests/test_new_nested2_FAIL.js
test_number_constructor_FAIL.js

Only tests that fail, but I guess unrelated?

Wait, I left debug print lines in there, gotta remove them, one sec.

[gfwilliams]

Looks great, thanks!

Only tests that fail, but I guess unrelated?

Yes, that's fine - I tagged the ones we expect to fail with _FAIL and the ones in manual shouldn't really be executed

[gfwilliams]

Argh, what a pain - it seems that this increases the code size by 800 bytes, so now the Puck and Pixl builds fail :(

I've had a bit of a look through and I've tried to reduce some duplication but I only managed to save 250 bytes, so now at least the Pixl build is working but the Puck one fails.

If we could save another 300 bytes it'd work, but I think I'll probably have to pull some features out to make this build again now

[d3nd3]

we could remove the support for thenables and less strict error checking in certain places. Dunno if that would be enough. The code is already quite compact, i'm surprised.

      if (isThenable) {
        JsVar *args[2] = {jsResolve,jsReject};
        JsExecFlags oldExecute = execInfo.execute;
        jsvUnLock(jspeFunctionCall(then, 0, data, false, 2, args));
        execInfo.execute = oldExecute;
        JsVar *exception = jspGetException();
        if (exception) {
          _jswrap_promise_reject(prombox, exception);
          jsvUnLock(exception);
        }
      } else {
        jsvUnLock(jswrap_promise_then(data,jsResolve,jsReject));
      }

That is the thenable code lines above.

Some of the strings can be reduced in length mb :
Executor function required in promise constructor
#define JS_PROMISE_ISRESOLVED_NAME JS_HIDDEN_CHAR_STR"resolved"

this child property renamed shorter?
jsvObjectSetChild(reaction, "nextBox", nextPromBox);

[gfwilliams]

Thanks - yes, I was a bit surprised too! 800b is quite a lot all things considered.

I had a bit of a dive through and I found some other places I could save a bit of space, so everything is now compiling again.

Good point about the names though - I'll lower them - I believe if we can get under 4 chars in total then key+value get stored in a single JsVar usually so they same RAM too.

Removing thenables is a good idea - although I'm sure someone will come along and complain at some point, but I'll comment it out for now and see.

[gfwilliams]

Actually we do have a test in there for thenables, so I guess we'll keep them for now.

With the other changes I made (reducing string sizes in the Puck.js lib and other stuff) we're back down to normal flash usage now