sibbl / hass-lovelace-kindle-screensaver

This tool generates a png from a Home Assistant Lovelace view, which can be displayed on a Kindle device which has the Online Screensaver plugin installed.
MIT License
315 stars 71 forks source link

Crash when connection is refused #12

Closed bictorv closed 4 months ago

bictorv commented 3 years ago

If the server to fetch the lovelace screen from isn't up, the hass-lovelace-kindle-screensaver docker process crashes, which is inconvenient. This happens for me when the host system (which runs both Home Assistant and h-l-k-s) reboots.

It would be better if it waits a minute and tries again, and repeats until successful.

(node:26) UnhandledPromiseRejectionWarning: Error: net::ERR_CONNECTION_REFUSED at http://XXXX:8123
    at navigate (/app/node_modules/puppeteer/lib/cjs/puppeteer/common/FrameManager.js:115:23)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async FrameManager.navigateFrame (/app/node_modules/puppeteer/lib/cjs/puppeteer/common/FrameManager.js:90:21)
    at async Frame.goto (/app/node_modules/puppeteer/lib/cjs/puppeteer/common/FrameManager.js:417:16)
    at async Page.goto (/app/node_modules/puppeteer/lib/cjs/puppeteer/common/Page.js:784:16)
    at async /app/index.js:36:3
(node:26) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:26) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
blockarchitech commented 3 years ago

If you can, submit the docker log. Also try seeing if your firewall (on the host) accepts both out and inbound traffic on udp and tcp w/ port 8123.

bictorv commented 3 years ago

That's what it was at the end (output from "docker logs"), or what kind of docker log do you mean?

The firewall etc has absolutely nothing to do with this - it's just that Home Assistant wasn't up and running when h-l-k-s started. When h-l-k-s is restarted everything works fine. What is needed is to catch the exception, sleep, and retry.

sibbl commented 2 years ago

@bictorv thanks for the report. Seems like I never had this issue as I have the nginx HASS addon running in front of Home Assistant. Perhaps this is a quick workaround as I currently don't have much time to fix this.

If the whole Docker container crashes (and not just some thread inside the Node.js app), then it would also be possible to run the docker container with --restart unless-stopped or add restart: unless-stopped to the docker-compose.yml. But I haven't tried this myself.

Karli2727 commented 2 years ago

Hi there,

I'm having the same issue and tried out several things, I solved it temporarily by doing a scheduled docker restart every hour to prevent freezing if I restart home-assistant for some reason (scheduled or just playing around).

A permanent solution would be really appreciated, either by acutally fixing it the way bictorv suggested or maybe easier by just letting the container crash when that error is thrown so that the docker restart policy can actually detect it and do the reboot that solves the issue. Would you consider that?

bictorv commented 2 years ago

If you figure out a way to crash the container (in javascript), I could do the coding.

sibbl commented 2 years ago

Unfortunately I didn't have the time yet to look into the issue for this project.

I assume that we might get a disconnected browser at some point, or can catch timeout exceptions somewhere. I've already had some experiences with this in another project. I assume the fix for this issue here is kind of similar: https://github.com/sibbl/blinkist-podcast-server/blob/master/src/utils/crawler.mjs#L25-L35

Karli2727 commented 2 years ago

that looks very promising, the error appears at a refused connection so it is safe to assume that you can catch it with this.browser.on("disconnected"

If you put it in the repo I'm happy to test it.

bictorv commented 2 years ago

I tried, but it seems "disconnected" is not catching this error (but other situations, so it's a good idea). Other suggestions?

Or does anyone know if it's possible to set the node CLI flag --unhandled-rejections=strict as indicated in the error message, through the puppeteer API? It seems it would make a lot of sense, also if its is going to be the default behaviour.

aptonline commented 2 years ago

I'm seeing a similar issue when I take the home assistant docker container down briefly to update hass-lovelace-kindle-screensaver crashes with the following in the log:

Error: net::ERR_CONNECTION_REFUSED at http://x.x.x.x:8123
stdout
12:14:19
    at navigate (/app/node_modules/puppeteer/lib/cjs/puppeteer/common/FrameManager.js:155:23)
stdout
12:14:19
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
stdout
12:14:19
    at async FrameManager.navigateFrame (/app/node_modules/puppeteer/lib/cjs/puppeteer/common/FrameManager.js:130:21)
stdout
12:14:19
    at async Frame.goto (/app/node_modules/puppeteer/lib/cjs/puppeteer/common/FrameManager.js:500:16)
stdout
12:14:19
    at async Page.goto (/app/node_modules/puppeteer/lib/cjs/puppeteer/common/Page.js:1167:16)
stdout
12:14:19
    at async /app/index.js:40:3
bictorv commented 2 years ago

My impression is that the problem is gone: the process crashes and is then restarted. The original error message above said

In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

and my guess is that we are now in that future.

Sometimes waiting will solve the problem. ;-)

lanrat commented 2 years ago

I've fixed this in my fork (as well as lots of other changes) by having the code sleep for a bit and then retry when it can't reach Home Assistant. I've found it greatly helps performance to not have the docker container constantly being restarted just because Home Assistant is down.

https://github.com/lanrat/hass-screenshot

aptonline commented 2 years ago

I've fixed this in my fork (as well as lots of other changes) by having the code sleep for a bit and then retry when it can't reach Home Assistant. I've found it greatly helps performance to not have the docker container constantly being restarted just because Home Assistant is down.

https://github.com/lanrat/hass-screenshot

Is this fork inline with recent changed in the main code? (battery reporting changes).

lanrat commented 2 years ago

Is this fork inline with recent changed in the main code? (battery reporting changes).

I changed all metadata being reported to the server to use HTTP headers instead of query parameters. And I use MQTT Discovery to automatically report the data to Home Assistant.

It also has a better index page with stats/info on the screenshots and devices connected.

aptonline commented 2 years ago

Does it rely on the screensaver extension for the battery reporting to work?

lanrat commented 2 years ago

I have a modified version of that too: https://github.com/lanrat/hass-kindle-screensaver

The readme has not been fully updated yet.

aptonline commented 2 years ago

I've fixed this in my fork (as well as lots of other changes) by having the code sleep for a bit and then retry when it can't reach Home Assistant. I've found it greatly helps performance to not have the docker container constantly being restarted just because Home Assistant is down.

https://github.com/lanrat/hass-screenshot

lanrat/hass-screenshot Image doesn't seem to be available on Docker Hub.

lanrat commented 2 years ago

Correct. You need to build it yourself. Just run make.

sibbl commented 2 years ago

Hey @lanrat, thanks for your fork and the changes you made there.

Do you see any chance that we can get features into this repository via a PR? Ideally without dropping any features so e.g. users can decide whether to use MQTT or HTTP reporting to Home Assistant?

I can understand that it was easier for you to make a fork and adapt it to your needs. However, I don't think most people will want to build their own docker image and make it available to their devices, as most just run Portainer on Hass.io itself. I think everyone can benefit from this if we can merge the best solutions together.

I'd appreciate if you think this is possible and would love to invest some time in merging pieces of your code. I for myself don't have that much time to develop this project in the next month, but would love to look over code and help testing so I can support this process as good as I can.

Let me know what you think and feel free to create a PR whenever you think it makes sense 😊 thank you!

lanrat commented 2 years ago

Hey @sibbl,

My original intent was to submit a few PRs. However since then, the scope of the changes increased and do contain some breaking changes, so I'm not sure how to proceed. Especially since my fork includes some changes you are not ready to include (ex: #7).

Unfortunately I don't have the time to go through and cherry pick commits to submit as PRs, especially since I also forked https://github.com/lanrat/hass-kindle-screensaver to work with the new changes as well.

If it's easiest for everyone, I can look into adding automated docker builds to my fork.

I'm happy to hear your thoughts on this.

aptonline commented 2 years ago

@sibbl , @lanrat as a user, and someone who has now tried both versions of each project, it would be really great to see a combination of both feature sets in one release but also do understand the time constraints for both of you.

@lanrat would it be possible to enable issue tracking on your repositories as although I have it installed there are a number things that I don’t have working (MQTT discovery for one) and I don’t feel it’s right posting about them here 🤣.

lanrat commented 2 years ago

@aptonline I just enabled issues. Thanks for letting me know!