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

add support for fetch timeout control for node_helpers, fix timeouts on armv6l #3660

Merged
merged 6 commits into from
Dec 25, 2024

Conversation

sdetweil
Copy link
Collaborator

@sdetweil sdetweil commented Dec 24, 2024

user reporting slow/no connection/timeout errors on armv6l for calendar, and newsfeed

we can increase the timeout by adding calls to the undici lib, but it requires node 20.18.1 or above.

this adds the support for timeout
(also environment variable to override if needed,, mmFetchTimeout (default 30 seconds)

and updates the base node version

@bugsounet
Copy link
Contributor

Warn: package-lock

integrity/resolve missing

@khassel
Copy link
Collaborator

khassel commented Dec 24, 2024

for changing the built-in node fetch timeout we have to add the whole undici http client as dependency?

I would prefer a simpler solution e.g. adding env var --network-family-autoselection-attempt-timeout (untestet)

@sdetweil
Copy link
Collaborator Author

sdetweil commented Dec 24, 2024

no idea what this drags in

const { setGlobalDispatcher, Agent } = require("undici");

but that is why I linked to the solution ahead of time..

@bugsounet
Copy link
Contributor

/!\ resolve and integrity are removed from package-lock.json

For information it's allows to download each libraries from an compressed file
Without it, MagicMiror deps will take many many Time for installing

@sdetweil
Copy link
Collaborator Author

@bugsounet I don't know what you are telling me.. git says there are two lines changed in package.json

systeminfo(changed version) and undici (added)

after I changed, I erased package-lock.json , and did a new npm install to get the latest

@khassel
Copy link
Collaborator

khassel commented Dec 24, 2024

@sdetweil how did you test this? With an arm v6 machine or can I simulate this some other way?

@sdetweil
Copy link
Collaborator Author

i tested on the armv6l that had the timeout error on both news and calendar and also on my amd64 machine.

@khassel
Copy link
Collaborator

khassel commented Dec 24, 2024

what url did you use for testing/simulating?

@sdetweil
Copy link
Collaborator Author

the default config urls. on armv6l
and my google cal and default on my amd

i had hard coded it in calendar to
prove, news failed

thats why the question about fixing for any/all via app/whatever
then put this on the armv6, and both worked
and of course all the testcases e2e and electron

@sdetweil
Copy link
Collaborator Author

sdetweil commented Dec 24, 2024

only way to simulate would be to create some cpu sucker threads to cause delays . armv6 problem is small memory and slow processor handling swapping on sd card

@sdetweil
Copy link
Collaborator Author

sdetweil commented Dec 24, 2024

i wonder if the guys 11 calendars failure would be fixed here
https://forum.magicmirror.builders/topic/19031/calendar-module-error-with-many-google-calendars?_=1734883587981

@khassel
Copy link
Collaborator

khassel commented Dec 24, 2024

@sdetweil my question was only how I can test it, used this not working url now.

Your solution works, my tests with --network-family-autoselection-attempt-timeout does not work. The undici package is small, so I'm fine with merging this beside the package-lock.json problem.

If you look at the diffs of package-lock.json you removed many many lines like

			"resolved": "https://registry.npmjs.org/@altano/repository-tools/-/repository-tools-0.1.1.tgz",
			"integrity": "sha512-5vbUs2A98CC3g1AlOBdkBE0BMukkLjLIsMHAtuxg6Pt9dQXxYWdLKOf66v6c/vIqtNcgTMv0oGtddLdMuH9X6w==",

Don't know how you did it but this must be reverted before merge ...

@sdetweil
Copy link
Collaborator Author

@khassel oh, how to test timeout
use this

https://httpbin.org/delay/10

the number is seconds of delay

@sdetweil
Copy link
Collaborator Author

If you look at the diffs of package-lock.json you removed many many lines like

I did NOT "remove" anything

I see the resolved is missing on some, and the integrity value is different.. I just ran npm install with no package-lock, and no node_modules folder to generate a new one , this IS a different node version..

npm install (not install-mm) on my amd64 is blazing fast, <30 seconds total

@khassel
Copy link
Collaborator

khassel commented Dec 24, 2024

I did NOT "remove" anything

look at the diff

@sdetweil
Copy link
Collaborator Author

@khassel I saw the diff.. but I didn't DO it on purpose.. just npm install

@khassel
Copy link
Collaborator

khassel commented Dec 24, 2024

seems to be a npm issue, maybe this helps: npm/cli#4263 (comment)

@sdetweil
Copy link
Collaborator Author

sdetweil commented Dec 24, 2024

I ran npm install again without removing the files and it looks better..
pushed

@bugsounet
Copy link
Contributor

Remove node_helper and do npm install again
Diff package-lock.json and see result.
Must be fine

@sdetweil
Copy link
Collaborator Author

package-lock.json did not change

@bugsounet
Copy link
Contributor

I can't verify from my computer, sorry

@sdetweil
Copy link
Collaborator Author

sdetweil commented Dec 24, 2024

I can't verify from my computer, sorry

I don't know what you are telling me

@bugsounet
Copy link
Contributor

@sdetweil : verified it's ok for me :)

package.json Outdated Show resolved Hide resolved
@khassel khassel merged commit 6a09bc4 into MagicMirrorOrg:develop Dec 25, 2024
11 checks passed
rejas pushed a commit that referenced this pull request Dec 27, 2024
continue from #3660 

Fix: package-lock.json (check node engine)
@sdetweil sdetweil mentioned this pull request Jan 1, 2025
sdetweil added a commit that referenced this pull request Jan 1, 2025
## [2.30.0] - 2025-01-01

Thanks to: @xsorifc28, @HeikoGr, @bugsounet, @khassel,
@KristjanESPERANTO, @rejas, @sdetweil.

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

### Added

- [core] Add wayland and windows start options to `package.json` (#3594)
- [docs] Add step for npm publishing in release process (#3595)
- [core] Add GitHub workflow to run spellcheck a few days before each
release (#3623)
- [core] Add test flag to `index.html` to pass to module js for test
mode detection (needed by #3630)
- [core] Add export on animation names (#3644)
- [compliments] Add support for refreshing remote compliments file, and
test cases (#3630)
- [linter] Re-add `eslint-plugin-import`now that it supports ESLint v9
(#3586)
- [linter] Re-activate `eslint-plugin-package-json` to lint
`package.json` (#3643)
- [linter] Add linting for markdown files (#3646)
- [linter] Add some handy ESLint rules.
- [calendar] Add ability to display end date for full date events, where
end is not same day (showEnd=true) (#3650)
- [core] Add text to the config.js.sample file about the locale variable
(#3654, #3655)
- [core] Add fetch timeout for all node_helpers (thru undici, forces
node 20.18.1 minimum) to help on slower systems. (#3660) (3661)

### Changed

- [core] Run code style checks in workflow only once (#3648)
- [core] Fix animations export #3644 only on server side (#3649)
- [core] Use project URL in fallback config (#3656)
- [core] Fix Access Denied crash writing js/positions.js (on synology
nas) #3651. new message, MM starts, but no modules showing (#3652)
- [linter] Switch to 'npx' for lint-staged in pre-commit hook (#3658)

### Removed

- [tests] Remove `node-pty` and `drivelist` from rebuilded test (#3575)
- [deps] Remove `@eslint/js` dependency. Already installed with `eslint`
in deep (#3636)

### Updated

- [repo] Reactivate `stale.yaml` as GitHub action to mark issues as
stale after 60 days and close them 7 days later (if no activity) (#3577,
#3580, #3581)
- [core] Update electron dependency to v32 (test electron rebuild) and
all other dependencies too (#3657)
- [tests] All test configs have been updated to allow full external
access, allowing for easier debugging (especially when running as a
container)
- [core] Run and test with node 23 (#3588)
- [workflow] delete exception `allow-ghsas: GHSA-8hc4-vh64-cxmj` in
`dep-review.yaml` (#3659)

### Fixed

- [updatenotification] Fix pm2 using detection when pm2 script is inside
or outside MagicMirror root folder (#3576) (#3605) (#3626) (#3628)
- [core] Fix loading node_helper of modules: avoid black screen, display
errors and continue loading with next module (#3578)
- [weather] Change default value for weatherEndpoint of provider
openweathermap to "/onecall" (#3574)
- [tests] Fix electron tests with mock dates, the mock on server side
was missing (#3597)
- [tests] Fix testcases with hard coded Date.now (#3597)
- [core] Fix missing `basePath` where `location.host` is used (#3613)
- [compliments] croner library changed filenames used in latest version
(#3624)
- [linter] Fix ESLint ignore pattern which caused that default modules
not to be linted (#3632)
- [core] Fix module path in case of sub/sub folder is used and use
path.resolve for resolve `moduleFolder` and `defaultModuleFolder` in
app.js (#3653)
- [calendar] Update to resolve issues #3098 #3144 #3351 #3422 #3443
#3467 #3537 related to timezone changes
- [calendar] Fix #3267 (styles array), also fixes event with both exdate
AND recurrence(and testcase)
- [calendar] Fix showEndsOnlyWithDuration not working, #3598, applies
ONLY to full day events
- [calendar] Fix showEnd for Full Day events (#3602)
- [tests] Suppress "module is not defined" in e2e tests (#3647)
- [calendar] Fix #3267 (styles array, really this time!)
- [core] Fix #3662 js/positions.js created incorrectly

---------

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: Karsten Hassel <[email protected]>
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: 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]>
Co-authored-by: HeikoGr <[email protected]>
Co-authored-by: Pedro Lamas <[email protected]>
Co-authored-by: veeck <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants