espruino / Espruino

The Espruino JavaScript interpreter - Official Repo
http://www.espruino.com/
Other
2.73k stars 741 forks source link

Promise handling rewrite break BLE device use #2495

Closed halemmerich closed 2 months ago

halemmerich commented 2 months ago

2v21.114 on Bangle 2 breaks bthrm and bthrmlite for me. It seems to boil down to NRF.requestDevice() not returning an actual promise and then failing because of the missing then-function. The promise itself seems to be run correctly since it prints unhandled errors like Uncaught Error: Unhandled promise rejection: No device found matching filters.

You can check with:

print(NRF.requestDevice({filters:[]}));

It prints { } on 2v21.114 while it prints Promise: { } on 2v21.

gfwilliams commented 2 months ago

Eurgh. Ok, thanks. This'll be because of #2454

Stuff like this in BLE is done with blePromise = jspromise_create(); and then to complete we do jspromise_resolve(blePromise, data); or similar.

But looking at this, jspromise_create just creates an empty object you've called box, so it's not a promise and nothing like .then could be used with it?

So I'm not sure how this can really work as-is... @d3nd3 any thoughts?

I guess maybe you assumed jspromise_create wasn't used outside so changed what it does? While we could just return the promise like jswrap_promise_constructor does, then the box is just not referenced at all and freed immediately which seems like it's not the plan?

d3nd3 commented 2 months ago

@gfwilliams I was aware of jspromise_create() usage outside, I thought its fine since I made the resolve/reject functions take a prombox instead of a promise, but if they are returning the promise to javascript usercode it has to be adjusted to return the child promise which it contains.

JsVar *promise = jsvLockAgainSafe(blePromise);
  jsble_central_getPrimaryServices(jswrap_ble_BluetoothRemoteGATTServer_getHandle(parent), uuid);
  return jsvObjectGetChildIfExists(blePromise, JS_HIDDEN_CHAR_STR"prom");

Can you validate the locks? Not my speciality.

Mb its good practise if we name the variables prombox to make it clear also. Box is just a naming for container. Also, are there any other functions which return a promise like this?

2454

gfwilliams commented 2 months ago

In the code snippet above, I'm not sure what's going on with JsVar *promise = jsvLockAgainSafe(blePromise); lock-wise, but in the actual PR as far as I could tell the locks seemed ok (and the tests do check for dangling locks, so the fact they pass is a good sign).

Also, are there any other functions which return a promise like this?

A few I think - it's any Espruino function that creates and returns something to the user - so off the top of my head, the bluetooth connect (maybe disconnect), write, startNotifications, then on Bangle we've got beep, buzz, pressure. etc.

Basically it's a pretty big bunch of changes. I'll try and reply properly in https://github.com/espruino/Espruino/pull/2454