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

Multiple Instances of Node Helper running for a single module #3502

Closed
taylornoss opened this issue Jul 12, 2024 · 4 comments
Closed

Multiple Instances of Node Helper running for a single module #3502

taylornoss opened this issue Jul 12, 2024 · 4 comments

Comments

@taylornoss
Copy link

I found a bug in MagicMirror

Platform: Electron version 31.1.0 running on Windows (reproducible on both Windows 10 and Windows 11)

Node Version: v20.15.1

MagicMirror² Version: 2.28.0

Description:
There are a couple of modules my mirror that are repeated, to be configured with different module level settings. I'm seeing instances of multiple node helpers being created for the repeated modules, which is leading to unnecessary noise in communication between the modules and the node helpers.

It's my understanding that there should only be one instance of a node helper per module type. From the Module Development Documentation:

For every module type, only one node helper instance will be created. For example: if your MagicMirror uses two calendar modules, there will be only one calendar node helper instantiated.

Here's the output from the command line with two instances of the MMM-GoogleCalendar module in the config file

[2024-07-12 17:59:28.017] [LOG]   Starting MagicMirror: v2.28.0
[2024-07-12 17:59:28.072] [LOG]   Loading config ...
[2024-07-12 17:59:28.073] [LOG]   config template file not exists, no envsubst
[2024-07-12 17:59:28.074] [LOG]   Loading module helpers ...
[2024-07-12 17:59:28.075] [LOG]   No helper found for module: alert.
[2024-07-12 17:59:28.077] [LOG]   Initializing new module helper ...
[2024-07-12 17:59:28.078] [LOG]   Module helper loaded: updatenotification
[2024-07-12 17:59:28.078] [LOG]   No helper found for module: clock.
[2024-07-12 17:59:28.683] [LOG]   Initializing new module helper ...
[2024-07-12 17:59:28.683] [LOG]   Module helper loaded: MMM-GoogleCalendar
[2024-07-12 17:59:28.684] [LOG]   Initializing new module helper ...
[2024-07-12 17:59:28.684] [LOG]   Module helper loaded: MMM-GoogleCalendar

Each instance of the module seems to be loading it's own version of the node helper.

Reading through the initialization code, I'm not seeing anything preventing duplicate node helpers from being started. I think the answer might be as simple as removing duplicates from the module list before loading them? We could just use a Set to remove any duplicates from the modules array:

// MagicMirror -> js -> app.js
// this.start function, line #272

// change 
await loadModules(modules);
// to 
let unique = [...new Set(modules)];
await loadModules(unique);

Or prevent duplicates from being added to the array further upstream, but this would just be a one-liner fix.

@bbf
Copy link

bbf commented Jul 17, 2024

I encountered the same bug while trying to understand why a module was not working properly and there a broadcast storm of WS messages. Is this a regression from the original behavior or was it always like this?

@bbf
Copy link

bbf commented Jul 17, 2024

I found the issue:
53fc814#r144318440

@sdetweil
Copy link
Collaborator

@sdetweil
Copy link
Collaborator

we changed to allowing an async process from mode_helper.start
but there is a race condition where we haven't completed the first and allow more.

@khassel khassel added the bug label Jul 17, 2024
rejas pushed a commit that referenced this issue Aug 4, 2024
…ces of a module are in config.js (#3517)

adds a check for already loaded/loading for node helper of a each
module, only does once

fixes #3502
rejas pushed a commit that referenced this issue Aug 18, 2024
…3523)

- [x] Base your pull requests against the `develop` branch.
- [x] Include these infos in the description:
> - Does the pull request solve a **related** issue?
Yes - solves #3521

> - If so, can you reference the issue like this `Fixes
#<issue_number>`?
Fixes #3521 (also mentioned in commit message)

> - What does the pull request accomplish? Use a list if needed.

> > - Updates duplicate module filter method (upstream vs downstream -
see #3502)
> > - Updates socket io catchall functionality to new API
[[docs](https://socket.io/docs/v4/listening-to-events/)].

- [x] Please run `npm run lint:prettier` before submitting
- [x] Don't forget to add an entry about your changes to the
CHANGELOG.md file.
khassel added a commit that referenced this issue Sep 30, 2024
## [2.29.0] - 2024-10-01

Thanks to: @bugsounet, @dkallen78, @jargordon, @khassel,
@KristjanESPERANTO, @MarcLandis, @rejas, @ryan-d-williams, @sdetweil,
@skpanagiotis.

> ⚠️ This release needs nodejs version `v20` or `v22`, minimum version
is `v20.9.0`

### Added

- [compliments] Added support for cron type date/time format entries mm
hh DD MM dow (minutes/hours/days/months and day of week) see
https://crontab.cronhub.io for construction (#3481)
- [core] Check config at every start of MagicMirror² (#3450)
- [core] Add spelling check (cspell): `npm run test:spelling` and handle
spelling issues (#3544)
- [core] removed `config.paths.vendor` (could not work because `vendor`
is hardcoded in `index.html`), renamed `config.paths.modules` to
`config.foreignModulesDir`, added variable `MM_CUSTOMCSS_FILE` which -
if set - overrides `config.customCss`, added variable `MM_MODULES_DIR`
which - if set - overrides `config.foreignModulesDir`, added test for
`MM_MODULES_DIR` (#3530)
- [core] elements are now removed from `index.html` when loading script
or stylesheet files fails
- [core] Added `MODULE_DOM_UPDATED` notification each time the DOM is
re-rendered via `updateDom` (#3534)
- [tests] added minimal needed node version to tests (currently v20.9.0)
to avoid releases with wrong node version info
- [tests] Added `node-libgpiod` library to electron-rebuild tests
(#3563)

### Removed

- [core] removed installer only files (#3492)
- [core] removed raspberry object from systeminformation (#3505)
- [linter] removed `eslint-plugin-import`, because it doesn't support
ESLint v9. We will reenter it later when it does.
- [tests] removed `onoff` library from electron-rebuild tests (#3563)

### Updated

- [weather] Updated `apiVersion` default from 2.5 to 3.0 (#3424)
- [core] Updated dependencies including stylistic-eslint
- [core] nail down `node-ical` version to `0.18.0` with exception
`allow-ghsas: GHSA-8hc4-vh64-cxmj` in `dep-review.yaml` (which should
removed after next `node-ical` update)
- [core] Updated SocketIO catch all to new API
- [core] Allow custom modules positions by scanning index.html for the
defined regions, instead of hard coded (PR #3518 fixes issue #3504)
- [core] Detail optimizations in `config_check.js`
- [core] Updated minimal needed node version in `package.json`
(currently v20.9.0) (#3559) and except for v21 (no security updates)
(#3561)
- [linter] Switch to ESLint v9 and flat config and replace
`eslint-plugin-unicorn` by `@eslint/js`
- [core] fix discovering module positions twice after #3450

### Fixed

- Fixed `checks` badge in README.md
- [weather] Fixed issue with the UK Met Office provider following a
change in their API paths and header info.
- [core] add check for node_helper loading for multiple instances of
same module (#3502)
- [weather] Fixed issue for respecting unit config on broadcasted
notifications
- [tests] Fixes calendar test by moving it from e2e to electron with
fixed date (#3532)
- [calendar] fixed sliceMultiDayEvents getting wrong count and
displaying incorrect entries, Europe/Berlin (#3542)
- [tests] ignore `js/positions.js` when linting (this file is created at
runtime)
- [calendar] fixed sliceMultiDayEvents showing previous day without
config enabled

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Michael Teeuw <[email protected]>
Co-authored-by: Kristjan ESPERANTO <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ross Younger <[email protected]>
Co-authored-by: Veeck <[email protected]>
Co-authored-by: Bugsounet - Cédric <[email protected]>
Co-authored-by: jkriegshauser <[email protected]>
Co-authored-by: illimarkangur <[email protected]>
Co-authored-by: sam detweiler <[email protected]>
Co-authored-by: vppencilsharpener <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: Paranoid93 <[email protected]>
Co-authored-by: Brian O'Connor <[email protected]>
Co-authored-by: WallysWellies <[email protected]>
Co-authored-by: Jason Stieber <[email protected]>
Co-authored-by: jargordon <[email protected]>
Co-authored-by: Daniel <[email protected]>
Co-authored-by: Ryan Williams <[email protected]>
Co-authored-by: Panagiotis Skias <[email protected]>
Co-authored-by: Marc Landis <[email protected]>
@khassel khassel closed this as completed Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants