[Bugfix] Fix PrometheusStatsLogger crash on re-instantiation and non-daemon thread#905
Open
SuperMarioYL wants to merge 1 commit intoModelEngine-Group:developfrom
Conversation
…daemon thread Two bugs in observability.py: 1. When _metric_mappings is already populated (e.g. multi-engine or hot-reload), __init__ returns early without initializing instance attributes (is_running, thread, config, etc.). Any subsequent call to shutdown() or __del__() raises AttributeError. Fix: always initialize core instance attributes before the early-return guard. 2. The update_stats_loop thread is not created as a daemon thread. If shutdown() is never explicitly called, this thread prevents the Python process from exiting cleanly. Fix: set daemon=True, and guard shutdown() against thread being None. Signed-off-by: supermario_leo <leo.stack@outlook.com>
SuperMarioYL
requested review from
FangRun2,
Tarrei,
mag1c-h and
ygwpz
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fix two functional bugs in
PrometheusStatsLoggerthat cause crashes in multi-engine scenarios and prevent clean process shutdown.Modifications
Bug 1:
AttributeErroron re-instantiationWhen
_metric_mappingsis already populated (e.g. multi-engine deployment or hot-reload),__init__returns early at line 71 before initializing any instance attributes (is_running,thread,config,log_interval, etc.).Any subsequent call to
shutdown()or the__del__destructor raisesAttributeError: 'PrometheusStatsLogger' object has no attribute 'is_running'.Fix: Move
self.config,self.log_interval,self.is_running, andself.threadinitialization before the early-return guard. The second instance becomes a safe no-op that can be cleanly shut down.Bug 2: Non-daemon
update_stats_loopthread blocks process exitThe background thread is created without
daemon=True. Ifshutdown()is never explicitly called (e.g. crash, unclean teardown,__del__not invoked by GC), this thread keeps the Python process alive indefinitely.Fix: Set
daemon=Trueon the thread. Also guardshutdown()withif self.thread is not Noneto handle the re-instantiation case safely.Test
black --checkandisort --check --profile=blackpass on the modified file.