clearWatch(undefined) & friends

[espruino-discuss]

Posted at 2019-05-10 by AkosLukacs

Hi!

Recently ran into an issue with clearWatch in library code. (This cost me couple of hours, so possibly over exaggerating this...)

clearWatch() and clearWatch(undefined) clears all pin-watches. You can accidentally call it with undefined, if you don't check whether you have an actual watch Id, for example line 31-32 in DHT22.js.

Because of the "reliability" of the DHT11 & 22 sensors and the 10 default retries in the libraries, these are dangerous for almost 6 seconds (550 ms timeout * 10). If you don't read the docs, you can run into a race condition where you start a conversion before all possible timeouts of the previous read had finished.
If you use setWatch elsewhere, that clearWatch(undefined) just destroys you program without crash, and leads to some head scratching... Or leads to a memory leak, if you call .read() too soon. It did leak slowly at 10 retries and 3 second read interval. At the end the setWatch 'count' was up to 50. But that's a different matter.

Also clearTimeout and clearInterval is the same.

First, a suggestion:
What would be the preferred way to handle clear***()? Seen several ways in devices / modules code: I think it shouldn't be without checks and should set the 'watch' property to falsey, if there is one.

For example there is this nice, but nontrivial looking example from the TouchRD code (_ is "this", and _.w is the watch Id):

_.w = _.w && clearWatch(_.w);

Or the more obvious

if (this.watch) {
     clearWatch(this.watch);
     this.watch = undefined;
}

If there is a consensus, I'm willing to change the libraries, around dozen uses in JS files.

Does anyone think it would worth making clearWatch(undefined) invalid, and changing the clear-all call explicit (for example pass in -1)? I know, it's a breaking change. Possibly write a warn to console?
Or better just improve the libraries & docs?

[espruino-discuss]

Posted at 2019-05-10 by @allObjects

Good practice - I use - is:

• define (global) variables for all your watches, timeouts, and in intervals you plan to set
• give the name a suffix, like xyzWId or xyzWHndl for watch handles
• initialized them with false at declaration (optional, any falsy evaluating value will do: undefined, null, 0, "", false)
• always use the variable when setting the Watch / Interval / Timeout
• always use the following construct when clearing a Watch / Interval / Timeout - mostly I use a function or method of an object where I have a simplified invocation interface to make sure I do not misspell the variable name and other things. Most of the time there is more that has to go on at the same time with the particular clear...(), and that goes nicely in such a function or method.... and some times these things have even to go conditioned on the ...Id as well.

  var xgzWId = false;
// ...
// ...
// ...
  xyzWid = setWatch(xyzFunction,pin,options);
// ...
// ...
// ...
   if (xyzWId) { xyzWId = clearWatch(xyzWId); }

PS: clear...() returns undefined, which is falsy and does the expected job.

[espruino-discuss]

Posted at 2019-05-12 by AkosLukacs

Thanks for the input!
That snippet looks good, it's short, and I think still clear, after you learn that clearXX returns undefined!

[espruino-discuss]

Posted at 2019-05-12 by @allObjects

Forgot to add that 'mistakes' I noticed are setting up (especially) setInterval(). For example, the uploading onto the board already sets one (which 'never ever should be done), then the code is saved, and for some logic reason on power up the setInterval() is called again when power is cycled. Therefore, the abcIId variable to hold the Id of the interval can be used too to prevent that. I the function is (the same and) with the same parameters, the setup() can be skipped. Otherwise, it has to be cleared and set again.

NB: If just the time of the interval changes, you do not need to clear and setup the function again: you can just change the interval:

changeInterval(abcIId,newTimeInMS); // change rime of running abc interval

[gfwilliams]

Posted at 2019-05-13 by @gfwilliams

As you say this could easily be a source of potential confusion. I'll see how hard it is to change clearXX such that they error if called with undefined

[gfwilliams]

Posted at 2019-05-13 by @gfwilliams

Ok, just fixed - it'll be in 2v03.

But yes, I always try and do if (interval) clearInterval(interval)