Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transitioning node-persist to node-persist 3.0 #629

Closed
jvmahon opened this issue Dec 10, 2018 · 23 comments
Closed

Transitioning node-persist to node-persist 3.0 #629

jvmahon opened this issue Dec 10, 2018 · 23 comments
Labels
discussion enhancement long running Issues which should be excluded from the stale bot

Comments

@jvmahon
Copy link

jvmahon commented Dec 10, 2018

This is a follow-up to an off-topic discussion originally started in https://github.com/KhaosT/HAP-NodeJS/issues/586 concerning transitioning node-persist from the legacy version to a more modern version. The issue with doing so as raised there was that node-persist provides no simple transitioning tools to move the database from the legacy data storage to updated version.

I am working on this type of data transitioning for another project and so I'm posting the basic technique here in case it is ever decided to transition HAP-nodeJS to an updated version.

In my case, I decided the "best" way to transition the data was to be able to run both the old (0.0.12) and new 3.0 node-persist side-by-side and run the data through a simple loop reading the old data using node-persist 0.0.12 and writing to a new database using node-persist 3.0. Another node-persist user gave me some advice on how to code this. See simonlast/node-persist#87

This technique requires that you have node persist 0.0.12 and 3.0 loaded at the same time. To enable this, I published 0.0.12 under a new npm package name node-persist-legacy. See https://github.com/jvmahon/node-persist-legacy. Then its just a matter of looping over the old data and re-writing it using the node-persist 3.0 calls.

Some further details on how the updated code would work . . .

In order to transition data, I decided that what I'd do is to store the updated persist 3.0 database in a folder "Persist3" which is a subfolder located in the "normal" persist database folder.

In the "main" body of code, only node persist 3.0 calls are used.

On startup, the program checks if either (1) the "Persist3" folder exists or (2) if neither the "Persist3" folder exists nor any legacy database exists. If either of these is true, then the data was already transitioned or there is no older data to worry about, so the data is already in a state where the persist 3.0 calls in the code are ready to be used.

If, however, there exists "old" node-persist 0.0.12 (or earlier) database, but there isn't a Persist3 folder, then this is the first-run with node-persist 3 and the legacy data needs to be copied to the new database. So, in this case, you run through a simple database copying loop as mentioned above (and further detailed in: https://github.com/jvmahon/node-persist-legacy) and copy the data.

@hassankhan
Copy link
Contributor

Could it be an idea to use the XDG Base Directory Specification? Essentially this would mean new node-persist configuration would live under ~/.config/hap-nodejs rather than in a folder inside the project.

@KhaosT
Copy link
Contributor

KhaosT commented Aug 24, 2019

@hassankhan technically whomever import hap-nodejs needs to provide the path for us to write the config, and the default fallback for the bundled persist library was the "current" location, so a lot of time when you run it directly, it generates the persist folder in the project itself 😅

If we are really thinking about migration, it might be worth just migrate to a SQLite db so we can get more flexibility down the road...

@jvmahon
Copy link
Author

jvmahon commented Aug 24, 2019

The idea of migrating to SQLite is interesting. On a related issue - one of the things I've been thinking about is the initial start-up speed of iOS Home when interfacing with Hap-NodeJS / Homebridge. I have over a hundred devices and, at times, there are long "updating" delays in iOS when starting Home. I was wondering if there is a lot of delay occurring due to synchronous operations (e.g., in their interactions with the iPhone or Apple TV) which might be lowered if there was more asynchronous processing; That's a reason for considering the updated node-persists it seems to favor an async model (as well as considering SQLite) - but I'm wondering if moving to more asynchronous transactions generally is what's needed. I.e., is it time to raise the level of the required nodeJS to say the 10 or 12 level so contributors could consider moving more functions to an asynchronous model? Not sure if this makes sense, but am interested in your thoughts.

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Aug 24, 2019 via email

@jvmahon
Copy link
Author

jvmahon commented Aug 24, 2019

As the request is sent to each HAP-NodeJS instance separately, you can improve the “updating” performance by putting all the quick responders in one instance, and the slow ones in a separate instance. This should change the home app to appear more responsive.

Yes, that's what I've done using HomeBridge - dividing different HomeBridge plugins into different instances helps a lot. This seems to support the thought that if the programming model itself was able to do more asynchronously, it would also improve performance but without requiring users to figure out how to divide devices into different executing instances. I don't know if this is something that could be done reasonably easily with HAP-NodeJS, but was interested in hearing the author's (KHaosT's) and other's thoughts on it.

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Aug 24, 2019 via email

@ebaauw
Copy link
Contributor

ebaauw commented Aug 24, 2019

deprecate the GET event interface, and force all plugins to use updateValue

Please don’t - this will break Eve history.

Also in homebridge-zp, I actually use this deliberately for the LED and button lock states. Sonos doesn’t provide notifications for these, and I don’t want to poll the zone players just for these two attributes. The Sonos app also does a just-in-time retrieval before displaying these attributes.

Would it make sense to score each plugin based on implementation, and usage of the get event interface. Gold would be ones that push status updates via updateValue in real time, silver would be ones that poll devices regularly and use updateValue, bronze would be ones that use the GET event to return device status.

I don’t think you can score a plugin. The same plugin might employ multiple schemes. homebridge-zp would be bronze for the LED and button lock state, but gold for everything else. homebridge-hue is silver for the Hue bridge, which doesn’t provide any notifications, but gold for deCONZ, which does.

In itself, GET is not the issue (see the Eve history case), but doing asynchronous I/O before calling callback. Here you should distinguish between guarding the I/O with a timeout (of 1 to 2 seconds) vs an I/O that might take longer.

@jdtsmith
Copy link

jdtsmith commented Jan 21, 2020

Just found this fascinating discussion @NorthernMan54 + @ebaauw. To clarify, if I do not provide a characteristic.on('get',... then whenever HomeKit needs a new value to display for that characteristic, it will automatically take one that HAP has cached for it (presumably from some earlier updateValue)? I always thought I had to provide that cached value myself (with simple event callbacks like getStatus(callback) {callback(this.status)}).

Is a sensible low-latency "gold" pattern for plugins then something like:

  • Avoid on('get',... callbacks, instead use characteristic.updateValue(...) not on demand, but whenever it is needed (via polling your device, or waiting for updates from it).
  • Provide an on('set',... callback, but
  • Avoid characteristic.setValue(..), preferring characteristic.updateValue( to reflect changes back to HomeKit without calling the set handling callback.

I can see @ebaauw's concern though: if you have some service reflected in HomeKit that is "informational only" (thinks a switch reporting some internal state of your device), which is not something your plugin will act on if its state changes, there is no sense polling for that information.

Sounds like HAP needs to accept, in addition to actual values, promises to provide an updated value. Then just update them as they resolve.

@ebaauw
Copy link
Contributor

ebaauw commented Jan 21, 2020

Using promises instead of callbacks won’t change the issue. The HomeKit runtime does a synchronous GET to the HAP server, blocking the HomeKit application until the HAP server has sent a response. Whether that response is reported through a callback function or by resolving a promise doesn’t change the fact that the HomeKit app is blocked. You would want to send a response immediately (by returning the cached value) or guard the asynchronous device I/O with a timeout and send the cached value in case the device I/O takes too long.

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Jan 21, 2020 via email

@jdtsmith
Copy link

Thanks, both. I'm still unclear on the difference between setValue and updateValue. Is it only that updateValue doesn't generate a 'set' event? If so, I can't see why a plugin would ever call setValue on one of its own services, since recursive loops could ensue.... i.e. it seems setValue is best called exclusively by HAP/Homebridge themselves.

And to be clear, is it true I can actually entirely omit a 'get' handler for a service characteristic as long as I am updateValue'ing regularly from external events (or don't mind polling)? The advantage even of intermittent polling is it is unlikely to collide with the Homekit GET from HAP.

BTW, do either of you know a good way to see which plugins are stalling out the HAP response to Homekit when the dreaded "updating" plagues the Home App? Like a Homebridge profiling log level ("XXXXXXX took 3.9s responding to status request for Service YYYY's Characteristic ZZZZ"). That's a better approach I think than GOLD/SILVER/BRONZE or whatever: just a shame board of who's holding up the process the longest and for which services/characteristics. Reminds me of the Battery Usage by App screen on the iPhone. Users would naturally filter this info back to plug-in developers...

@jdtsmith
Copy link

Using promises instead of callbacks won’t change the issue. The HomeKit runtime does a synchronous GET to the HAP server, blocking the HomeKit application until the HAP server has sent a response.

I see. An obvious solution is just to cutoff waiting for all callbacks after some user-configurable delay, falling back to the old cached values. Some things might not be correct, but at least it wouldn't block the whole system...

@jvmahon
Copy link
Author

jvmahon commented Jan 21, 2020

Thanks, both. I'm still unclear on the difference between setValue and updateValue. Is it only that updateValue doesn't generate a 'set' event? If so, I can't see why a plugin would ever call setValue on one of its own services, since recursive loops could ensue.... i.e. it seems setValue is best called exclusively by HAP/Homebridge themselves.

And to be clear, is it true I can actually entirely omit a 'get' handler for a service characteristic as long as I am updateValue'ing regularly from external events (or don't mind polling)? The advantage even of intermittent polling is it is unlikely to collide with the Homekit GET from HAP.

BTW, do either of you know a good way to see which plugins are stalling out the HAP response to Homekit when the dreaded "updating" plagues the Home App? Like a Homebridge profiling log level ("XXXXXXX took 3.9s responding to status request for Service YYYY's Characteristic ZZZZ"). That's a better approach I think than GOLD/SILVER/BRONZE or whatever: just a shame board of who's holding up the process the longest and for which services/characteristics. Reminds me of the Battery Usage by App screen on the iPhone. Users would naturally filter this info back to plug-in developers...

I'm not 100% sure of this (this is from memory), but I think a difference between setValue and updateValue is that if you have a .on ('change', function()... ) set up, and you call .setValue, that will then cause a change to emit and your .on ('change', function()... ) will trigger. If you use .updateValue the .on ('change', function()... ) does not trigger.

@Supereg
Copy link
Member

Supereg commented Jan 21, 2020

Both of them trigger a change event (given the value actually changed). Only setValue triggers the set event and is the method use by hap-nodejs itself to set the value coming from a HAP request (and it really doesn't make sense that plugins call this function).

@jvmahon
Copy link
Author

jvmahon commented Jan 21, 2020

hem t

That sounds right. Thanks for the correction.

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Jan 22, 2020

@jdtsmith @jvmahon Back on your plugin scoring comment, I think we should deprecate the 'get' event with the next major upgrade. It would force everyone to improve their plugin device status logic, and get rid of the "updating" issue permanently.

Am thinking that the update to the next version of HAP-NodeJS is likely to require rework of everyone's plugin, so it would be a good time to make the move.

@ebaauw
Copy link
Contributor

ebaauw commented Jan 22, 2020

I think we should deprecate the 'get' event with the next major upgrade.

Please don’t, as this will break the fakegato history. The issue isn’t the get event, the issue is doing I/O in the event handler before returning control.

@jdtsmith
Copy link

I think we should deprecate the 'get' event with the next major upgrade.

Please don’t, as this will break the fakegato history. The issue isn’t the get event, the issue is doing I/O in the event handler before returning control.

If you are not producing a status value through just-in-time I/O or network access, why could you not updateValue?

@Supereg
Copy link
Member

Supereg commented Jan 22, 2020

I think we should deprecate the 'get' event with the next major upgrade.

This would render any control point characteristics unusable (and would probably be problematic for any other characteristic, which do not support events).

@NorthernMan54
Copy link
Contributor

NorthernMan54 commented Jan 22, 2020 via email

@stale
Copy link

stale bot commented Jun 16, 2020

This issue has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this issue was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

@stale stale bot added the stale label Jun 16, 2020
@Supereg Supereg pinned this issue Jun 16, 2020
@stale stale bot removed the stale label Jun 16, 2020
@Supereg Supereg unpinned this issue Jun 22, 2020
@stale
Copy link

stale bot commented Aug 15, 2020

This issue has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this issue was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

@stale stale bot added the stale label Aug 15, 2020
@stale stale bot closed this as completed Aug 22, 2020
@Supereg Supereg reopened this Aug 22, 2020
@stale stale bot removed the stale label Aug 22, 2020
@Supereg Supereg pinned this issue Oct 6, 2020
@stale
Copy link

stale bot commented Oct 21, 2020

This issue has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this issue was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement long running Issues which should be excluded from the stale bot
Projects
None yet
Development

No branches or pull requests

8 participants