fix some test problems#1583
Merged
Merged
Conversation
Reviewer's GuideRefactors APIResponseCache database initialization to be lazy and loop-agnostic to avoid pytest-asyncio cancellation issues, aligns tests with the project’s SQLite helper, and gates aiohttp connector’s enable_cleanup_closed option on Python version to work around a socket cleanup bug only where needed. Sequence diagram for lazy APIResponseCache initialization on first getsequenceDiagram
actor Caller
participant APIResponseCache
participant SQLite
Caller->>APIResponseCache: get(provider, artist_name, endpoint, params)
activate APIResponseCache
APIResponseCache->>APIResponseCache: _ensure_initialized()
activate APIResponseCache
alt cache not initialized
APIResponseCache->>APIResponseCache: _initialize_db()
activate APIResponseCache
APIResponseCache->>SQLite: create tables
SQLite-->>APIResponseCache: success
APIResponseCache->>APIResponseCache: set _initialized = True
deactivate APIResponseCache
else cache already initialized
APIResponseCache-->>APIResponseCache: skip initialization
end
deactivate APIResponseCache
APIResponseCache->>SQLite: SELECT cached response
SQLite-->>APIResponseCache: row or none
APIResponseCache-->>Caller: cached response or None
deactivate APIResponseCache
Class diagram for lazy APIResponseCache initializationclassDiagram
class APIResponseCache {
+cache_dir pathlib_Path
+db_file pathlib_Path
-__lock asyncio_Lock
-__lock_loop asyncio_AbstractEventLoop
-_initialized bool
+__init__(cache_dir pathlib_Path)
+_initialize_db() async
+_ensure_initialized() async
+_handle_missing_table_error(operation str) async
+get(provider str, artist_name str, endpoint str, params dict) async
+put(provider str, artist_name str, endpoint str, params dict, response_data bytes, status_code int, content_type str, expires_in int) async
+cleanup_expired() async int
+get_cache_stats() async dict
+clear_cache(provider str) async
+close() async
}
Flow diagram for create_http_connector configuration by Python versionflowchart TD
A[create_http_connector with service_type and ssl_context] --> B[Set base_config with ssl and keepalive_timeout]
B --> C{Python version < 3.13.12?}
C -->|Yes| D[Set base_config.enable_cleanup_closed = True]
C -->|No| E[Do not set enable_cleanup_closed]
D --> F{service_type == musicbrainz?}
E --> F
F -->|Yes| G[Apply stricter connection limits for MusicBrainz]
F -->|No| H[Use default connection limits]
G --> I[Create and return TCPConnector]
H --> I[Create and return TCPConnector]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The lazy initialization via
_ensure_initializedcan race if multiple coroutines call it concurrently when_initializedis stillFalse, leading to multiple_initialize_dbruns; consider guarding the initialization flag with a lock or anasyncio.Eventso that only one initializer runs and others await its completion. - Since
_ensure_initializedis now the gatekeeper for DB readiness, you could update_handle_missing_table_errorto call_ensure_initializedinstead of_initialize_dbdirectly, keeping the initialization logic and invariants centralized in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The lazy initialization via `_ensure_initialized` can race if multiple coroutines call it concurrently when `_initialized` is still `False`, leading to multiple `_initialize_db` runs; consider guarding the initialization flag with a lock or an `asyncio.Event` so that only one initializer runs and others await its completion.
- Since `_ensure_initialized` is now the gatekeeper for DB readiness, you could update `_handle_missing_table_error` to call `_ensure_initialized` instead of `_initialize_db` directly, keeping the initialization logic and invariants centralized in one place.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1583 +/- ##
==========================================
- Coverage 60.77% 60.73% -0.04%
==========================================
Files 124 124
Lines 18979 18982 +3
==========================================
- Hits 11534 11529 -5
- Misses 7445 7453 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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
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.
Summary by Sourcery
Lazily initialize the API cache database and align SQLite usage and HTTP connector configuration to improve reliability in tests and runtime.
Bug Fixes:
Enhancements:
Tests: