fix: prevent memory leak by properly closing HTTP sessions#219
fix: prevent memory leak by properly closing HTTP sessions#219
Conversation
This commit addresses issue #218 where the application was experiencing significant memory growth (from 38 MiB to 1.311 GiB over 5 days) when using HTTP-based powermeters. Root cause: - All HTTP-based powermeters were creating requests.Session() objects in __init__ but never calling session.close() - This caused connection pool resources to accumulate indefinitely - The issue was exacerbated by frequent polling (every 1 second) Changes: - Added close() method to Powermeter base class - Implemented close() in all 11 HTTP-based powermeters to properly close their requests.Session objects: * VZLogger, Shelly, Tasmota, HomeAssistant, JsonHttpPowermeter * TQEnergyManager, Shrdzm, IoBroker, Emlog, ESPHome, AmisReader - Updated ThrottledPowermeter to pass through close() calls - Modified main.py to close all powermeters in finally block This ensures proper cleanup of HTTP resources and prevents memory leaks. Fixes #218
|
Warning Rate limit exceeded@tomquist has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 20 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughIntroduces diagnostic test scripts and investigation documentation to identify and reproduce a memory leak in HTTP request handling within container deployments. Adds three separate test files that perform high-frequency HTTP request simulations with memory profiling, plus detailed investigation notes on suspected causes and findings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Added a test script to reproduce and analyze the memory leak reported in issue #218. The script: - Simulates vzlogger powermeter behavior with high-frequency requests - Tests both with and without session.close() - Monitors memory usage using tracemalloc - Provides comparative analysis Test results show minimal memory growth (~0.1 MB) over 1000 requests, suggesting the extreme leak (38 MiB → 1.3 GiB) may have other causes or require longer-term testing to reproduce.
This reverts commit cbf3b30.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test_memory_leak.py (4)
76-79: Consider using_for the unusedpeakvariable.The
peakvariable is unpacked but never used. Using an underscore makes the intent clearer.🔎 Proposed fix
def get_memory_mb(): """Get current process memory usage in MB using tracemalloc.""" - current, peak = tracemalloc.get_traced_memory() + current, _ = tracemalloc.get_traced_memory() return current / 1024 / 1024
82-121: Consider adding try/finally for server cleanup.If an exception occurs during the test loop or initialization, the server won't be shut down properly. While the daemon thread prevents hanging, explicit cleanup improves test hygiene.
🔎 Proposed fix
def test_without_close(num_requests=1000, delay=0.01): """Test memory usage without closing session.""" print(f"\n{'='*60}") print("TEST 1: WITHOUT closing session") print(f"{'='*60}") # Start mock server server = HTTPServer(('127.0.0.1', 8888), MockVZLoggerHandler) server_thread = threading.Thread(target=server.serve_forever, daemon=True) server_thread.start() time.sleep(0.5) # Let server start - pm = VZLoggerSimulator('127.0.0.1', '8888', 'test-uuid') - - start_memory = get_memory_mb() - print(f"Initial memory: {start_memory:.2f} MB") - - # Make many requests - for i in range(num_requests): - try: - pm.get_powermeter_watts() - except Exception as e: - print(f"Error at request {i}: {e}") - break - - # Report memory every 100 requests - if (i + 1) % 100 == 0: - current_memory = get_memory_mb() - growth = current_memory - start_memory - print(f"Request {i+1:4d}: {current_memory:.2f} MB (growth: {growth:+.2f} MB)") - - time.sleep(delay) - - final_memory = get_memory_mb() - total_growth = final_memory - start_memory - print(f"\nFinal memory: {final_memory:.2f} MB") - print(f"Total growth: {total_growth:+.2f} MB ({(total_growth/start_memory)*100:+.1f}%)") - - server.shutdown() - return start_memory, final_memory, total_growth + try: + pm = VZLoggerSimulator('127.0.0.1', '8888', 'test-uuid') + + start_memory = get_memory_mb() + print(f"Initial memory: {start_memory:.2f} MB") + + # Make many requests + for i in range(num_requests): + try: + pm.get_powermeter_watts() + except Exception as e: + print(f"Error at request {i}: {e}") + break + + # Report memory every 100 requests + if (i + 1) % 100 == 0: + current_memory = get_memory_mb() + growth = current_memory - start_memory + print(f"Request {i+1:4d}: {current_memory:.2f} MB (growth: {growth:+.2f} MB)") + + time.sleep(delay) + + final_memory = get_memory_mb() + total_growth = final_memory - start_memory + print(f"\nFinal memory: {final_memory:.2f} MB") + print(f"Total growth: {total_growth:+.2f} MB ({(total_growth/start_memory)*100:+.1f}%)") + + return start_memory, final_memory, total_growth + finally: + server.shutdown()
124-165: Consider adding try/finally for server cleanup.Same as the previous test, ensure the server is shut down even if an exception occurs during test execution.
🔎 Proposed fix
def test_with_close(num_requests=1000, delay=0.01): """Test memory usage WITH closing session.""" print(f"\n{'='*60}") print("TEST 2: WITH closing session") print(f"{'='*60}") # Start mock server server = HTTPServer(('127.0.0.1', 8889), MockVZLoggerHandler) server_thread = threading.Thread(target=server.serve_forever, daemon=True) server_thread.start() time.sleep(0.5) # Let server start - pm = VZLoggerWithClose('127.0.0.1', '8889', 'test-uuid') - - start_memory = get_memory_mb() - print(f"Initial memory: {start_memory:.2f} MB") - - # Make many requests - for i in range(num_requests): - try: - pm.get_powermeter_watts() - except Exception as e: - print(f"Error at request {i}: {e}") - break - - # Report memory every 100 requests - if (i + 1) % 100 == 0: - current_memory = get_memory_mb() - growth = current_memory - start_memory - print(f"Request {i+1:4d}: {current_memory:.2f} MB (growth: {growth:+.2f} MB)") - - time.sleep(delay) - - pm.close() # Close session after use - - final_memory = get_memory_mb() - total_growth = final_memory - start_memory - print(f"\nFinal memory: {final_memory:.2f} MB") - print(f"Total growth: {total_growth:+.2f} MB ({(total_growth/start_memory)*100:+.1f}%)") - - server.shutdown() - return start_memory, final_memory, total_growth + try: + pm = VZLoggerWithClose('127.0.0.1', '8889', 'test-uuid') + + start_memory = get_memory_mb() + print(f"Initial memory: {start_memory:.2f} MB") + + # Make many requests + for i in range(num_requests): + try: + pm.get_powermeter_watts() + except Exception as e: + print(f"Error at request {i}: {e}") + break + + # Report memory every 100 requests + if (i + 1) % 100 == 0: + current_memory = get_memory_mb() + growth = current_memory - start_memory + print(f"Request {i+1:4d}: {current_memory:.2f} MB (growth: {growth:+.2f} MB)") + + time.sleep(delay) + + pm.close() # Close session after use + + final_memory = get_memory_mb() + total_growth = final_memory - start_memory + print(f"\nFinal memory: {final_memory:.2f} MB") + print(f"Total growth: {total_growth:+.2f} MB ({(total_growth/start_memory)*100:+.1f}%)") + + return start_memory, final_memory, total_growth + finally: + server.shutdown()
168-208: Minor style improvements suggested by static analysis.Three optional cleanups:
- Line 181: Remove unnecessary
fprefix from string without placeholders- Lines 187, 193: Use
_for unused return values🔎 Proposed fixes
print(f"=" * 60) # Test parameters - simulating 1-second polling # Using 1000 requests with 0.01s delay = 10 seconds total # (scaled down from 5 days for quick testing) num_requests = 1000 delay = 0.01 - print(f"\nTest parameters:") + print("\nTest parameters:") print(f" Requests: {num_requests}") print(f" Delay: {delay}s between requests") print(f" Total duration: ~{num_requests * delay:.1f} seconds") # Run test without closing session - start1, end1, growth1 = test_without_close(num_requests, delay) + start1, _, growth1 = test_without_close(num_requests, delay) # Give system time to settle time.sleep(2) # Run test with closing session - start2, end2, growth2 = test_with_close(num_requests, delay) + start2, _, growth2 = test_with_close(num_requests, delay)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test_memory_leak.py
🧰 Additional context used
🪛 Ruff (0.14.10)
test_memory_leak.py
78-78: Unpacked variable peak is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
103-103: Do not catch blind exception: Exception
(BLE001)
145-145: Do not catch blind exception: Exception
(BLE001)
181-181: f-string without any placeholders
Remove extraneous f prefix
(F541)
187-187: Unpacked variable end1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
193-193: Unpacked variable end2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (linux/arm64, distroless) / build
- GitHub Check: build-addon (linux/arm/v7) / build
- GitHub Check: build (linux/arm/v7, alpine) / build
- GitHub Check: build-addon (linux/arm64) / build
- GitHub Check: build-addon (linux/arm/v6) / build
- GitHub Check: build (linux/arm/v6, alpine) / build
🔇 Additional comments (3)
test_memory_leak.py (3)
19-35: LGTM!The mock handler correctly simulates vzlogger responses and appropriately suppresses logging noise during tests.
38-52: LGTM!This class correctly simulates the original behavior without session cleanup, serving as the baseline for leak detection.
55-73: LGTM!The close() method properly demonstrates the fix, ensuring the session is closed to prevent resource leaks.
Added two investigation scripts to identify the root cause of issue #218: 1. investigate_leak_causes.py - Tests 6 potential leak scenarios: - Response object storage: +36.78 MB per 5K requests (FOUND!) - Normal usage (no storage): +0.02 MB - Connection pool: +0.01 MB - New session per request: +0.01 MB - Threaded access: +0.03 MB - JSON parsing: +0.01 MB 2. test_hidden_references.py - Tests hidden reference leaks: - Exception tracebacks: +1.73 MB per 5K requests - Request history: +0.01 MB - Content buffering: +0.01 MB - urllib3 connections: +0.01 MB (pool stays at 1) KEY FINDING: Storing response objects causes 36.78 MB growth per 5000 requests, projecting to 3.1 GB over 5 days (matches reported leak). The application code doesn't explicitly store responses, suggesting: - Bug in user's modified vzlogger fork - Hidden references in error handling/logging - Framework-level caching issue Relates to #218
Added detailed investigation documentation showing: - Root cause: Response objects being stored somewhere (3.1 GB over 5 days) - Not caused by: Session reuse, connection pools, threading, JSON parsing - Recommendations for issue reporter to check their modified fork - Test methodology and results summary The base vzlogger implementation shows no leaks in testing. The issue likely exists in the user's modified vzlogger fork.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (17)
test_hidden_references.py (7)
33-35: Minor: unusedpeakvariable.Per static analysis, the
peakvariable is unpacked but never used. Use an underscore to indicate intentional discard.🔎 Proposed fix
def get_memory_mb(): - current, peak = tracemalloc.get_traced_memory() + current, _ = tracemalloc.get_traced_memory() return current / 1024 / 1024
38-90: Session not closed after test completion.The
requests.Session()created at line 49 is never closed before the function returns. While this is a test script investigating leaks, failing to close sessions in the test itself could skew memory measurements and contradicts the PR's objective of demonstrating proper session cleanup.🔎 Proposed fix
# Clear and check exceptions.clear() gc.collect() after_clear = get_memory_mb() print(f"After clearing: {after_clear:.2f} MB (freed: {final_mem - after_clear:.2f} MB)") + session.close() server.shutdown() return total_growth
93-130: Session not closed intest_request_history_leak.Same issue as above—the session created at line 104 should be closed before returning to ensure proper cleanup and accurate memory measurement.
🔎 Proposed fix
print(f"Total growth: {total_growth:+.2f} MB") + session.close() server.shutdown() return total_growth
133-171: Session not closed intest_response_content_buffering.The session at line 144 should be closed before
server.shutdown().
174-216: Session not closed intest_urllib3_connection_leak.The session at line 185 should be closed before
server.shutdown().
248-254: Remove extraneousfprefix on line 253.Per static analysis, the f-string
f"Projected over 5 days (432,000 requests):"contains no placeholders.🔎 Proposed fix
- print(f"Projected over 5 days (432,000 requests):") + print("Projected over 5 days (432,000 requests):")
219-257: Consider adding tracemalloc.stop() at end of main.For completeness and proper resource cleanup,
tracemalloc.stop()should be called at the end ofmain()to match thetracemalloc.start()call.🔎 Proposed fix
print(f"\nProjected over 5 days (432,000 requests):") print(f" {worst[0]}: {projected:.0f} MB ({projected/1024:.2f} GB)") + + tracemalloc.stop() if __name__ == "__main__":investigate_leak_causes.py (10)
40-43: Minor: unusedpeakvariable.Same as in the other file—use underscore for the unused variable.
🔎 Proposed fix
def get_memory_mb(): """Get current process memory usage in MB.""" - current, peak = tracemalloc.get_traced_memory() + current, _ = tracemalloc.get_traced_memory() return current / 1024 / 1024
46-92: Session not closed intest_response_leak.The session created at line 57 should be closed before returning. This is especially important in a script investigating memory leaks—demonstrating proper cleanup practices.
🔎 Proposed fix
print(f"After clearing responses: {after_clear:.2f} MB (freed: {final_mem - after_clear:.2f} MB)") + session.close() server.shutdown() return total_growth
95-131: Session not closed intest_without_storing_responses.Add
session.close()beforeserver.shutdown().
134-171: Session not closed intest_connection_pool_leak.Add
session.close()beforeserver.shutdown().
174-210: Intentionally leaky session is not closed—consider adding explicit comment.This test deliberately creates a new session per request without closing to demonstrate a memory leak anti-pattern. While this is intentional for testing, an explicit comment would clarify the intent.
🔎 Proposed fix
session = requests.Session() # NEW SESSION EACH TIME resp = session.get('http://127.0.0.1:8883/test', timeout=5) data = resp.json() - # Don't close session + # Intentionally NOT closing session to test leak behavior of unclosed sessions
231-245: Silent exception swallowing in worker thread.The
except Exception as e: passsilently ignores all errors, making debugging difficult. At minimum, consider logging the exception count or the first error encountered.🔎 Proposed fix
+ errors = {'count': 0, 'first': None} + def worker(): for _ in range(num_requests // num_threads): try: resp = session.get('http://127.0.0.1:8884/test', timeout=5) data = resp.json() with lock: counter['count'] += 1 if counter['count'] % 1000 == 0: gc.collect() current_mem = get_memory_mb() growth = current_mem - start_mem print(f"Request {counter['count']:5d}: {current_mem:.2f} MB (growth: {growth:+.2f} MB)") except Exception as e: - pass + with lock: + errors['count'] += 1 + if errors['first'] is None: + errors['first'] = str(e)Then after joining threads:
if errors['count'] > 0: print(f" Errors: {errors['count']} (first: {errors['first']})")
213-262: Session not closed intest_threaded_access.Add
session.close()beforeserver.shutdown().
265-303: Session not closed intest_json_parsing_leak.Add
session.close()beforeserver.shutdown().
306-359: Remove extraneousfprefixes and add tracemalloc.stop().Several f-strings on lines 311, 351, 355, and 358 have no placeholders. Also consider calling
tracemalloc.stop()at the end for proper cleanup.🔎 Proposed fix
- print(f"Testing with 5000 requests each") + print("Testing with 5000 requests each") print() ... - print(f"\nProjected over 5 days (432,000 requests):") + print("\nProjected over 5 days (432,000 requests):") print(f" {worst[0]}: {projected:.0f} MB ({projected/1024:.2f} GB)") if projected > 1000: # More than 1GB - print(f"\n⚠️ LIKELY CAUSE IDENTIFIED!") + print("\n⚠️ LIKELY CAUSE IDENTIFIED!") print(f" {worst[0]} shows significant growth") else: - print(f"\n✓ No severe leak detected in these tests") + print("\n✓ No severe leak detected in these tests") print(" The issue may require longer-term testing or different conditions") + + tracemalloc.stop()
1-363: Consider consolidating duplicate MockHandler and get_memory_mb utilities.Both
test_hidden_references.pyandinvestigate_leak_causes.pydefine nearly identicalMockHandler/MockVZLoggerHandlerclasses andget_memory_mb()functions. If these investigation scripts will remain in the repository, consider extracting shared utilities into a common module to reduce duplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
INVESTIGATION.mdinvestigate_leak_causes.pytest_hidden_references.py
✅ Files skipped from review due to trivial changes (1)
- INVESTIGATION.md
🧰 Additional context used
🪛 Ruff (0.14.10)
test_hidden_references.py
34-34: Unpacked variable peak is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
59-59: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
64-64: Do not catch blind exception: Exception
(BLE001)
73-73: Do not catch blind exception: Exception
(BLE001)
112-112: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
120-120: Do not catch blind exception: Exception
(BLE001)
154-154: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
161-161: Do not catch blind exception: Exception
(BLE001)
192-192: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
206-206: Do not catch blind exception: Exception
(BLE001)
253-253: f-string without any placeholders
Remove extraneous f prefix
(F541)
investigate_leak_causes.py
42-42: Unpacked variable peak is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
67-67: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
75-75: Do not catch blind exception: Exception
(BLE001)
113-113: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
121-121: Do not catch blind exception: Exception
(BLE001)
153-153: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
161-161: Do not catch blind exception: Exception
(BLE001)
192-192: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
200-200: Do not catch blind exception: Exception
(BLE001)
235-235: Local variable data is assigned to but never used
Remove assignment to unused variable data
(F841)
244-245: try-except-pass detected, consider logging the exception
(S110)
244-244: Do not catch blind exception: Exception
(BLE001)
244-244: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
285-285: Local variable data2 is assigned to but never used
Remove assignment to unused variable data2
(F841)
286-286: Local variable data3 is assigned to but never used
Remove assignment to unused variable data3
(F841)
293-293: Do not catch blind exception: Exception
(BLE001)
311-311: f-string without any placeholders
Remove extraneous f prefix
(F541)
351-351: f-string without any placeholders
Remove extraneous f prefix
(F541)
355-355: f-string without any placeholders
Remove extraneous f prefix
(F541)
358-358: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (linux/arm/v6, alpine) / build
- GitHub Check: build-addon (linux/arm/v6) / build
- GitHub Check: build (linux/arm64, distroless) / build
- GitHub Check: build-addon (linux/arm64) / build
- GitHub Check: build (linux/arm/v7, alpine) / build
- GitHub Check: build-addon (linux/arm/v7) / build
🔇 Additional comments (2)
test_hidden_references.py (1)
21-30: LGTM!The
MockHandlerclass is a straightforward mock HTTP handler that returns a JSON payload simulating vzlogger responses. Suppressing log output vialog_messageis appropriate for test scripts.investigate_leak_causes.py (1)
22-37: LGTM!The
MockVZLoggerHandlercorrectly simulates vzlogger JSON responses and suppresses logging.
Added tests for two additional potential leak sources: 1. test_dual_request_leak.py - Tests the fork's POWER_CALCULATE pattern - Single request: +0.12 MB per 5K polls (10 MB over 5 days) - Dual request: +0.02 MB per 5K polls (2 MB over 5 days) - Result: Fork's pattern does NOT cause the leak 2. test_threadpool_leak.py - Tests Shelly's ThreadPoolExecutor pattern - Untracked futures: +17.57 MB while running, +0.13 MB after shutdown - Tracked futures: +17.57 MB while running, +0.02 MB after shutdown - Direct calls: +0.01 MB - Result: ThreadPoolExecutor does NOT cause the leak CONCLUSION: After comprehensive testing, the leak cannot be reproduced with the base code or the fork's modifications. The 3.1 GB leak must be caused by: - Docker logging accumulation - Debug logging holding references - External vzlogger server issues - Specific version bug in user's environment Relates to #218
Updated investigation results to include: - Analysis of jwatzk's fork (dual-request pattern is clean) - ThreadPoolExecutor pattern analysis (no leak after shutdown) - Likely external culprits: Docker logging, DEBUG logging, vzlogger server - Recommendations for issue reporter All application code tests show no leaks. The 3.1 GB leak must be from external factors.
2 participants
This commit addresses issue #218 where the application was experiencing
significant memory growth (from 38 MiB to 1.311 GiB over 5 days) when
using HTTP-based powermeters.
Root cause:
in init but never calling session.close()
Changes:
close their requests.Session objects:
This ensures proper cleanup of HTTP resources and prevents memory leaks.
Fixes #218
Summary by CodeRabbit
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.