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

Async node_helper start() #2487

Closed
MikeBishop opened this issue Mar 17, 2021 · 12 comments · Fixed by #2928
Closed

Async node_helper start() #2487

MikeBishop opened this issue Mar 17, 2021 · 12 comments · Fixed by #2928

Comments

@MikeBishop
Copy link
Contributor

If a node_helper uses an async method for start(), it appears that MagicMirror declares the system ready as soon as the function returns the initial Promise. It would be nice if MM would collect the promises from the various modules, then await Promise.all(...) of them.

Steps to Reproduce: Use the async/await pattern within a node_helper's start() method

Expected Results: Initialization completes when all node_helpers are done initializing

Actual Results: MagicMirror reports ready while node_helper is still printing loading status to console

@rejas
Copy link
Collaborator

rejas commented Mar 17, 2021

Is there a module you can point to that uses async start?

@MikeBishop
Copy link
Contributor Author

I've been trying to use it in my own module, MMM-Powerwall. I borrowed logic from MMM-RemoteControl that reads config files off the disk, and I'd already used the async file access methods elsewhere.

@sdetweil
Copy link
Collaborator

but u only need to use async functions if u intend to use await. which is similar to using sync calls.
there are a few different approaches to using async and await without doing it with start.
you can start a timer, or use the notification call from module to node helper.

@rejas
Copy link
Collaborator

rejas commented Oct 3, 2022

Hi @MikeBishop finally came around to look at some old issues, and whipped up a PR for your suggestion. Seems to work on my local setup but I want to get a little feedback first before I proceed.

@rejas rejas self-assigned this Oct 6, 2022
@rejas rejas linked a pull request Oct 7, 2022 that will close this issue
rejas added a commit that referenced this issue Oct 13, 2022
In response to #2487 this implements a Promise.all for the node_helper
start calls

Co-authored-by: veeck <michael@veeck.de>
@rejas
Copy link
Collaborator

rejas commented Oct 29, 2022

Fixed with #2928

@rejas rejas closed this as completed Oct 29, 2022
MichMich added a commit that referenced this issue Jan 1, 2023
## [2.22.0] - 2023-01-01

Thanks to: @angeldeejay, @buxxi, @dariom, @dWoolridge,
@KristjanESPERANTO, @MagMar94, @naveensrinivasan, @retroflex, @SkySails
and @tom.

Special thanks to @khassel, @rejas and @sdetweil for taking over most
(if not all) of the work on this release as project collaborators. This
version would not be there without their effort. Thank you!

### Added

- Added test for remoteFile option in compliments module
- Added hourlyWeather functionality to Weather.gov weather provider
- Removed weatherEndpoint definition from weathergov.js (not used)
- Added css class names "today" and "tomorrow" for default calendar
- Added Collaboration.md
- Added new github action for dependency review (#2862)
- Added a WeatherProvider for Open-Meteo
- Added Yr as a weather provider
- Added config options "ignoreXOriginHeader" and
"ignoreContentSecurityPolicy"

### Removed

- Removed usage of internal fetch function of node until it is more
stable

### Updated

- Cleaned up test directory (#2937) and jest config (#2959)
- Wait for all modules to start before declaring the system ready
(#2487)
- Updated e2e tests (moved `done()` in helper functions) and use es6
syntax in all tests
- Updated da translation
- Rework weather module
- Make sure smhi provider api only gets a maximum of 6 digits
coordinates (#2955)
  - Use fetch instead of XMLHttpRequest in weatherprovider (#2935)
  - Reworked how weatherproviders handle units (#2849)
  - Use unix() method for parsing times, fix suntimes on the way (#2950)
  - Refactor conversion functions into utils class (#2958)
- The `cors`-method in `server.js` now supports sending and recieving
HTTP headers
- Replace `&hellip;` by `…`
- Cleanup compliments module
- Updated dependencies including electron to v22 (#2903)

### Fixed

- Correctly show apparent temperature in SMHI weather provider
- Ensure updatenotification module isn't shown when local is _ahead_ of
remote
- Handle node_helper errors during startup (#2944)
- Possibility to change FontAwesome class in calendar, so icons like
`fab fa-facebook-square` works.
- Fix cors problems with newsfeed articles (as far as possible), allow
disabling cors per feed with option `useCorsProxy: false` (#2840)
- Tests not waiting for the application to start and stop before
starting the next test
- Fix electron tests failing sometimes in github workflow
- Fixed gap in clock module when displayed on the left side with
displayType=digital
- Fixed playwright issue by upgrading to v1.29.1 (#2969)

Signed-off-by: naveen <172697+naveensrinivasan@users.noreply.github.com>
Co-authored-by: Karsten Hassel <hassel@gmx.de>
Co-authored-by: Malte Hallström <46646495+SkySails@users.noreply.github.com>
Co-authored-by: Veeck <github@veeck.de>
Co-authored-by: veeck <michael@veeck.de>
Co-authored-by: dWoolridge <dwoolridge@charter.net>
Co-authored-by: Johan <jojjepersson@yahoo.se>
Co-authored-by: Dario Mratovich <dario_mratovich@hotmail.com>
Co-authored-by: Dario Mratovich <dario.mratovich@outlook.com>
Co-authored-by: Magnus <34011212+MagMar94@users.noreply.github.com>
Co-authored-by: Naveen <172697+naveensrinivasan@users.noreply.github.com>
Co-authored-by: buxxi <buxxi@omfilm.net>
Co-authored-by: Thomas Hirschberger <47733292+Tom-Hirschberger@users.noreply.github.com>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: Andrés Vanegas Jiménez <142350+angeldeejay@users.noreply.github.com>
@sdetweil sdetweil reopened this Jan 21, 2023
@MikeBishop
Copy link
Contributor Author

So there's apparently an issue with waiting for modules that take a long time. It looks like if initialization of the modules gets pushed out past a certain point in electron's loading process, electron will display a blank screen locally. Accessing with a browser works fine, because everything initialized by then.

I think I have a clean repro of the issue in https://github.com/MikeBishop/MMM-asyncdeath – the synchronous start function produces this in the logs and loads normally:

[21.01.2023 14:55.02.692] [LOG]   Synchronous start-up -- all is well
[21.01.2023 14:55.02.693] [LOG]   Sockets connected & modules started ...
[21.01.2023 14:55.03.235] [LOG]   Launching application.

The async start function produces this in the logs and fails:

[21.01.2023 14:52.54.143] [LOG]   About to break by using await
[21.01.2023 14:52.54.884] [LOG]   Launching application.
[21.01.2023 14:52.56.512] [LOG]   Sockets connected & modules started ...

Note the reversed order of “Launching application” and “modules started”. Seems feasible this could also happen with a synchronous module startup that takes a long time, but I haven’t tested that.

Seems like there's an additional step needed here to delay Electron's startup until the modules are done loading.

@rejas
Copy link
Collaborator

rejas commented Jan 21, 2023

A quick look before bedtime makes me think the problem only occurs when starting MM with electron and not when in servermode?

@rejas
Copy link
Collaborator

rejas commented Jan 21, 2023

Seems like electron.js calls createWindow (when the electron app is "ready") but config isnt yet set (in the start callback at the end) due to the async nature....

@MikeBishop
Copy link
Contributor Author

A quick look before bedtime makes me think the problem only occurs when starting MM with electron and not when in servermode?

Correct -- web browsers connecting once everything finishes startup are fine. I think it's probably that electron launch fails (or fails to connect) and then never retries. If so, perhaps rather than delaying electron start-up, we could kick electron to retry once the backend is ready.

@rejas
Copy link
Collaborator

rejas commented Jan 22, 2023

I dont like delaying stuff arbitrarily since we dont know how long some modules might take. Maybe my PR solution is enough and a little more future-proof

khassel pushed a commit that referenced this issue Jan 26, 2023
Async node_helper dont have to finish immediately in loadModules. So the
start callback in the app.js with the config isnt called for some time.
But the electron ready event can already be fired in the meantime.

This lead to the electron app starting but without a config (which is
provded by the node_helper callback) therefor crashing.

This PR fixes #2487 by moving the callback call out of the loadModules
block, therefor the config is provided in time.

If any new async node_helper doesnt like this, we will see it :-)

Co-authored-by: veeck <michael@veeck.de>
@rejas
Copy link
Collaborator

rejas commented Feb 3, 2023

@MikeBishop did you have time to check out the fixed develop branch?

@rejas rejas added this to the 2.23 milestone Feb 18, 2023
@MikeBishop
Copy link
Contributor Author

Belated, but I reverted the work-around in my module and cherry-picked in this change, and MM is able to start up properly.

MichMich added a commit that referenced this issue Apr 4, 2023
## [2.23.0] - 2023-04-04

Thanks to: @angeldeejay, @buxxi, @CarJem, @dariom, @DaveChild, @dWoolridge, @grenagit, @Hirschberger, @KristjanESPERANTO, @MagMar94, @naveensrinivasan, @nfogal, @psieg, @rajniszp, @retroflex, @SkySails and @tomzt.

Special thanks to @khassel, @rejas and @sdetweil for taking over most (if not all) of the work on this release as project collaborators. This version would not be there without their effort. Thank you guys! You are awesome!

### Added

- Added increments for hourly forecasts in weather module (#2996)
- Added tests for hourly weather forecast
- Added possibility to ignore MagicMirror repo in updatenotification module
- Added Pirate Weather as new weather provider (#3005)
- Added possibility to use your own templates in Alert module
- Added error message if `<modulename>.js` file is missing in module folder to get a hint in the logs (#2403)
- Added possibility to use environment variables in `config.js` (#1756)
- Added option `pastDaysCount` to default calendar module to control of how many days past events should be displayed
- Added thai language to alert module
- Added option `sendNotifications` in clock module (#3056)

### Removed

- Removed darksky weather provider
- Removed unneeded (and unwanted) '.' after the year in calendar repeatingCountTitle (#2896)

### Updated

- Use develop as target branch for dependabot
- Update issue template, contributing doc and sample config
- The weather modules clearly separates precipitation amount and probability (risk of rain/snow)
  - This requires all providers that only supports probability to change the config from `showPrecipitationAmount` to `showPrecipitationProbability`.
- Update tests for weather and calendar module
- Changed updatenotification module for MagicMirror repo only: Send only notifications for `master` if there is a tag on a newer commit
- Update dates in Calendar widgets every minute
- Cleanup jest coverage for patches
- Update `stylelint` dependencies, switch to `stylelint-config-standard` and handle `stylelint` issues, update `main.css` matching new rules
- Update Eslint config, add new rule and handle issue
- Convert lots of callbacks to async/await
- Revise require imports (#3071 and #3072)

### Fixed

- Fix wrong day labels in envcanada forecast (#2987)
- Fix for missing default class name prefix for customEvents in calendar
- Fix electron flashing white screen on startup (#1919)
- Fix weathergov provider hourly forecast (#3008)
- Fix message display with HTML code into alert module (#2828)
- Fix typo in french translation
- Yr wind direction is no longer inverted
- Fix async node_helper stopping electron start (#2487)
- The wind direction arrow now points in the direction the wind is flowing, not into the wind (#3019)
- Fix precipitation css styles and rounding value
- Fix wrong vertical alignment of calendar title column when wrapEvents is true (#3053)
- Fix empty news feed stopping the reload forever
- Fix e2e tests (failed after async changes) by running calendar and newsfeed tests last
- Lint: Use template literals instead of string concatenation
- Fix default alert module to render HTML for title and message
- Fix Open-Meteo wind speed units
@khassel khassel closed this as completed Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants