Daemonize addon store, timeout web requests #16183
Conversation
seanbudd
requested review from
michaelDCurran
and removed request for
a team
seanbudd
changed the title
source/_addonStore/dataManager.py
Outdated
|
|
||
| def terminate(self): | ||
| if self._getAddonsThread.is_alive(): | ||
| self._getAddonsThread.join(timeout=FETCH_TIMEOUT_S + 1) |
There was a problem hiding this comment.
Having NvDA hang on exit for up to 120 seconds does not sound that great.
In deed, restarting NVDA, the new NvDA would kill it off after 10 seconds anyway.
What would be the consequence of not joining the thread here, and just having it killed off (as it is now a daemon thread) on process exit?
If it is important that we have the ability to clean up after requests/get etc returns on terminate, then we could look at putting every requests.get call in its own daemon thread which we would normally block on, but would have the ability to abandon if necessary.
There was a problem hiding this comment.
I've changed this timeout to 1. It's probably fine not to join, but if we reset config and reinitialize submodules the thread should be killed before firing the next one on initialization.
See test results for failed build of commit 2d8d7f1a32 |
May fix most cases of nvaccess#16123 and nvaccess#16122 Summary of the issue: requests.get has no default timeout, meaning a connection may hang until it is closed by the server. The add-on store uses requests with no timeout to fetch data from the add-on store, and download add-ons. This can cause threads to hang, preventing NVDA from exiting correctly. Update check also has similar risks with usage of urllib like requests. Many threads started in NVDA should also be daemonized. Description of user facing changes NVDA should exit properly in more cases, make installs safer Description of development approach Add timeouts for web requests and daemonize all threads in NVDA.
Link to issue number:
May fix most cases of #16123 and #16122
Summary of the issue:
requests.gethas no default timeout, meaning a connection may hang until it is closed by the server.The add-on store uses
requestswith no timeout to fetch data from the add-on store, and download add-ons.This can cause threads to hang, preventing NVDA from exiting correctly.
Update check also has similar risks with usage of
urlliblikerequests.Many threads started in NVDA should also be daemonized.
Description of user facing changes
NVDA should exit properly in more cases, make installs safer
Description of development approach
Add timeouts for web requests and daemonize all threads in NVDA.
Testing strategy:
Smoke tested running the installer and using the add-on store
Known issues with pull request:
None
Code Review Checklist: