[keyman] Remove exception in Keyman on uninitialized platform#3704
[keyman] Remove exception in Keyman on uninitialized platform#3704AryanMishra1789 wants to merge 1 commit intochaoss:mainfrom
Conversation
MoralCode
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
Id like to learn more about how adding a try/catch around the redis initialization relates to what you mentioned in the PR description regarding how keyman crashes if a key is fetched before being published.
keyman/Orchestrator.py
Outdated
| except Exception: | ||
| conn = None |
There was a problem hiding this comment.
Im not sure if swallowing/hiding the exception here is the best way to handle this
There was a problem hiding this comment.
Thanks for pointing this out.
The core fix in this PR is the change in new_key() to prevent Keyman from crashing when a key request arrives before platform state is initialized. The try/catch around Redis initialization was added defensively during debugging and isn’t essential to addressing the InvalidRequest crash.
I can remove that change and keep this PR focused solely on the key request handling.
Signed-off-by: AryanMishra17003 <158508418+AryanMishra17003@users.noreply.github.com>
19ff9a7 to
b073e41
Compare
|
I have removed the redis try/catch and updated the PR to reflect the requested change. |
|
Sounds good! i'd like to talk to John (the maintainer who wrote keyman) to see whether removing this exception is likely the best way to handle this underlying issue, or whether something else (like adding a try catch around the invocation in keyclient) would be better |
Description
This PR prevents Keyman from crashing during startup when a key request is received for a platform whose key pools have not yet been initialized.
After state cleanup or service restarts, workers may request keys before any keys have been published, which previously caused Keyman to raise InvalidRequest and crash.
This PR fixes #3700
Notes for Reviewers
This change treats the condition as a recoverable startup scenario rather than an invalid request. The fix is intentionally minimal and only adds a warning log while preventing the crash.
Signed commits