Espruino.Core.Serial.getPorts can callback too early

[rikkuness]

I've been trying to track down why Espruino.Core.Serial.getPorts was not returning local serial devices on my system (Arch Linux). What I discovered was that callback was being called before ports is being fully populated, even calling again with shouldCallAgain it appears to be actually a race condition, the ports are being returned by the device but not until after the callback is hit.

[gfwilliams]

Do you know why the original code isn't working?

devices.forEach(function (device) {
  // ...
  responses++;
  if (responses == devices.length) {
    ...

Seemed pretty clear-cut to me? Unless one of the serial implementations is calling callback twice? In which case we ought to fix that...

it appears to be actually a race condition, the ports are being returned by the device but not until after the callback is hit.

Surely this should be some issue in the node_serialport implementation?

The idea is that we will return whatever ports we can almost immediately as some users were getting fed up we were for instance waiting for Bluetooth devices when they just wanted to connect to serial and it was right there - but the handling for that (returning the ports as/when available and using shouldCallAgain) should be in each implementation?

[rikkuness]

Oooh that's good context, maybe I should dig further into the cause then, I'll add some debugging into callbacks and see what hits. I'd never seen this happen anywhere else to be fair, on my Mac etc. I do get ports back, but for whatever reason on my Linux machine I was always getting empty sets of ports back from the callback on getPorts even after multiple calls. That being said the only device I have right now is a PipBoy too, I should probably pick up some other devices to make investigating easier 😄

[gfwilliams]

Thanks! Worst case you could grab an ESP32 devkit or similar - they're usually only $5 or so

[rikkuness]

I've got an STM32 dev kit and an ESP32 dev kit, but I have also just ordered a Pixl.JS and an Espruino WiFi too to add to the collection 😄 I'll do some more digging on this either way rather than changing everything in case it's an issue elsewhere.

[rikkuness]

Alright got time to investigate this further and I've tracked down the issue I think.

The early callback is indeed one of the implementations calling the callback twice, it's the node_ble one! https://github.com/espruino/EspruinoTools/blob/master/core/serial_node_ble.js#L125-L129

In my case I hit an error on scanDevices (because org.bluez.Error.NotReady), the .catch catches the error, calls the callback with an empty array, returns the callback and then calls the .then anyway and returns undefined

My patch in #195 works just because that changes the behavior in a way that rather than waiting for n responses, it's mapping responses to make sure it gets at least one from each device, which does feel better in some respects but still of course doesn't fix the root issue.

I'll push a fix for the actual issue now. Is there any benefit in changing to a promises type thing like I did in #195 or shall I drop that one and just fix the current issue?

[gfwilliams]

Great - thanks for looking into this and getting that fix done!

I'm all for moving to a promise-based solution (as given what just happened it seems more stable). However I'm not sure I understand why you're having to use Promise.allSettled? Couldn't we just use Promise.all() and not have to check promise.state (which feels a bit hacky)?

The serial endpoints should all call callback eventually - I guess one thing do to is to have a setTimeout (maybe 5s?) that reports an error if one of the endpoints hasn't returned with anything in that time? But right now that's probably not a big deal as I think everything should return ok.

[rikkuness]

I think originally I was thinking I'd have some reporting or logging if any rejected but I think Promise.all is a totally fine way to do it anyway, each implementation should have it's own log anyway right.

I guess I could also wrap each promise in the Promise.all in a Promise.race with a setTimeout so they all resolve in 5s, just some may resolve with timeout? Promises all the way down 😂

[gfwilliams]

Thanks - Promise.race inside would be quite tidy.

I think for now I wouldn't worry though - just the Promise.all would tidy things up nicely, and it doesn't do timeouts at the moment (and we haven't needed it to date) so maybe it's best not to complicate it.

In some ways it's better if it 'just breaks' and someone reports it rather than it breaking silently and taking 5s to get ports each time.

[rikkuness]

So I suppose the downside of Promise.all is that if any of the children reject, then the whole outer promise rejects, at the moment I don't know that matters too much because there is no internal rejection as each getPorts implementation has no error callback. Does that seem about right?

I just took delivery of my Pico and Pixl.js boards too so I'll try this with all the Espruino boards I have hooked up 😄

[gfwilliams]

Yes, that sounds good to me. Because we're just using callbacks, not Promises, I can't think we'd really end up doing any rejection (and if we ever did to that could just be caught and turned into an error + resolve with no ports?)

[rikkuness]

Sounds good to me yeah. I've updated #195 with the latest code 👌🏻