Move read_text to executor
#5688
Conversation
mdegat01
added
the
refactor
|
Warning Rate limit exceeded@agners has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 34 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 (47)
📝 WalkthroughWalkthroughThe pull request refactors numerous parts of the codebase to align with an asynchronous execution model. Most changes involve replacing direct, synchronous calls to error capturing (using Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Calling Component
participant Func as Async Function
participant Executor as Executor Context
participant Sentry as capture_exception
Component->>Func: Invoke async method
Func->>Executor: await sys_run_in_executor(capture_exception, error)
Executor->>Sentry: Execute capture_exception(error)
Sentry-->>Executor: Exception logged
Executor-->>Func: Return control asynchronously
Func-->>Component: Complete async error handling
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
supervisor/jobs/__init__.py (1)
193-196: Change in error handling approachThe direct call to
capture_exception()has been removed, and instead the_LOGGER.criticalis being passed as a parameter toRuntimeError. This changes how exceptions are logged and processed in this error path.This approach appears to be part of a broader strategy to move potentially blocking operations to executors, though in this specific case it's taking a different approach by using the logger directly.
Ensure that the error is still being properly captured elsewhere in the exception handling flow, possibly by a global exception handler that processes the logger parameter.
supervisor/utils/log_format.py (1)
16-16: Typographical nitpick in docstring.
Consider replacing “formated” with “formatted” in the docstring to maintain clarity.supervisor/utils/dbus.py (1)
127-129: Broad exception handling.
Catching all exceptions here is likely intentional for a final fallback. If possible, consider narrowing the exception scope or adding specialized error handling for better maintainability.supervisor/jobs/decorator.py (1)
376-376: Updated to use async free space method.The code now properly awaits
free_space()as it has been changed from a synchronous property to an asynchronous method.However, there's a potential issue in the error message on line 382. It still references
coresys.sys_host.info.free_spaceas if it were a property:- f"'{method_name}' blocked from execution, not enough free space ({coresys.sys_host.info.free_space}GB) left on the device" + f"'{method_name}' blocked from execution, not enough free space ({await coresys.sys_host.info.free_space()}GB) left on the device"supervisor/api/store.py (3)
188-193: Consider concurrency limiting with large numbers of add-ons.
Usingasyncio.gatherfor generating addon information concurrently will improve performance. However, if the store grows significantly, this could create too many tasks at once. Consider concurrency-limiting solutions if the store can hold hundreds of add-ons.
204-209: Reapply concurrency-limiting consideration here.
Similar to the previous comment aboutstore_info, generating an extensive add-ons list concurrently may require a concurrency limit if the add-on count grows.
287-289: Reading text is appropriate if documentation is purely text.
If the documentation includes non-text content, consider binary mode or a separate function that can distinguish file types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
supervisor/__main__.py(1 hunks)supervisor/addons/addon.py(1 hunks)supervisor/addons/manager.py(2 hunks)supervisor/addons/model.py(1 hunks)supervisor/api/__init__.py(1 hunks)supervisor/api/addons.py(1 hunks)supervisor/api/host.py(1 hunks)supervisor/api/middleware/security.py(1 hunks)supervisor/api/store.py(11 hunks)supervisor/api/utils.py(5 hunks)supervisor/backups/manager.py(2 hunks)supervisor/bootstrap.py(2 hunks)supervisor/core.py(3 hunks)supervisor/dbus/network/__init__.py(2 hunks)supervisor/docker/addon.py(2 hunks)supervisor/docker/interface.py(2 hunks)supervisor/hardware/disk.py(2 hunks)supervisor/homeassistant/core.py(4 hunks)supervisor/host/apparmor.py(1 hunks)supervisor/host/info.py(1 hunks)supervisor/jobs/__init__.py(1 hunks)supervisor/jobs/decorator.py(2 hunks)supervisor/misc/filter.py(1 hunks)supervisor/misc/tasks.py(2 hunks)supervisor/mounts/manager.py(1 hunks)supervisor/mounts/mount.py(4 hunks)supervisor/os/data_disk.py(2 hunks)supervisor/os/manager.py(2 hunks)supervisor/plugins/audio.py(1 hunks)supervisor/plugins/base.py(1 hunks)supervisor/plugins/cli.py(1 hunks)supervisor/plugins/dns.py(1 hunks)supervisor/plugins/manager.py(3 hunks)supervisor/plugins/multicast.py(1 hunks)supervisor/plugins/observer.py(1 hunks)supervisor/resolution/check.py(1 hunks)supervisor/resolution/checks/dns_server.py(1 hunks)supervisor/resolution/checks/dns_server_ipv6.py(1 hunks)supervisor/resolution/checks/free_space.py(2 hunks)supervisor/resolution/evaluate.py(1 hunks)supervisor/resolution/fixup.py(1 hunks)supervisor/resolution/notify.py(1 hunks)supervisor/store/data.py(1 hunks)supervisor/store/repository.py(2 hunks)supervisor/supervisor.py(1 hunks)supervisor/utils/apparmor.py(2 hunks)supervisor/utils/common.py(2 hunks)supervisor/utils/dbus.py(1 hunks)supervisor/utils/log_format.py(2 hunks)supervisor/utils/sentry.py(1 hunks)supervisor/utils/yaml.py(2 hunks)tests/homeassistant/test_home_assistant_watchdog.py(1 hunks)tests/host/test_info.py(1 hunks)tests/mounts/test_manager.py(2 hunks)tests/utils/test_log_format.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- supervisor/utils/yaml.py
- supervisor/utils/apparmor.py
- supervisor/utils/common.py
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in doc...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
supervisor/supervisor.pysupervisor/core.pysupervisor/bootstrap.pysupervisor/__main__.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
supervisor/supervisor.pysupervisor/core.pysupervisor/bootstrap.pysupervisor/__main__.py
`*/**(html|markdown|md)`: - Be brief in your replies and don...
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
supervisor/supervisor.pysupervisor/core.pysupervisor/bootstrap.pysupervisor/__main__.py
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
supervisor/supervisor.pysupervisor/core.pysupervisor/bootstrap.pysupervisor/__main__.py
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
supervisor/supervisor.pysupervisor/core.pysupervisor/bootstrap.pysupervisor/__main__.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
supervisor/supervisor.pysupervisor/core.pysupervisor/bootstrap.pysupervisor/__main__.py
🔇 Additional comments (93)
tests/host/test_info.py (1)
8-12: Properly updated test to use async/await pattern.The test has been correctly modified to match the updated asynchronous API of the
free_spacemethod. The function is now declared as async and properly awaits the result.supervisor/plugins/cli.py (1)
117-117: Properly moved error capturing to executor.The change correctly moves the
capture_exceptioncall to run in an executor, preventing blocking behavior in the main asynchronous event loop during error handling.supervisor/resolution/fixup.py (1)
60-60: Properly moved error capturing to executor.The change correctly moves the
capture_exceptioncall to run in an executor, preventing blocking behavior in the main asynchronous event loop during error handling.supervisor/mounts/manager.py (1)
180-180: Properly moved error capturing to executor.The change correctly moves the
capture_exceptioncall to run in an executor, preventing blocking behavior in the main asynchronous event loop during error handling.supervisor/resolution/checks/dns_server_ipv6.py (1)
50-50: Good asynchronous pattern implementationThe change moves the
capture_exceptioncall to run in an executor, which prevents blocking the main event loop when capturing exceptions. This is a good practice for potentially I/O-bound operations.supervisor/plugins/dns.py (1)
413-413: Appropriate use of executor for error captureMoving
capture_exceptionto run in an executor is a good practice. This prevents the main event loop from being blocked during error processing, especially if the error capture involves I/O operations.supervisor/resolution/checks/dns_server.py (1)
45-45: Well-implemented asynchronous error handlingThe modification correctly moves the exception capture to an executor, consistent with the pattern applied in other files. This ensures non-blocking error handling.
supervisor/store/data.py (1)
77-80: Improved docstring clarityThe added docstring note explicitly states that this function must be run in an executor, which is valuable information for developers. This helps prevent potential blocking of the event loop if the function is called incorrectly.
supervisor/plugins/observer.py (1)
124-124: Good change - error handling moved to executorMoving the capture_exception call to run in an executor is a good improvement that helps maintain the asynchronous nature of the code, preventing potential blocking operations during error handling.
supervisor/misc/filter.py (1)
83-85: Good change - disk free space retrieval updatedThis change looks good. Replacing the direct property access with an explicit method call to get disk free space fits with the PR's goal of moving operations like this to controlled contexts.
supervisor/supervisor.py (1)
222-222: Good change - error capture moved to executorMoving the Sentry error capture to run in an executor is a beneficial change. It prevents blocking the main event loop during exception handling in the update process.
supervisor/plugins/audio.py (1)
166-166: Good change - error handling moved to executorMoving the capture_exception call to run in an executor is consistent with the pattern applied to other similar methods throughout the codebase. This improves the async behavior of the repair method.
tests/homeassistant/test_home_assistant_watchdog.py (1)
144-144: Increased sleep duration for test stability.The sleep duration has been increased from 0 to 0.1 seconds, which likely helps avoid race conditions when testing asynchronous behavior, especially now that more operations are running in executors.
supervisor/__main__.py (1)
57-57: Properly awaiting the now-asynchronous supervisor_debugger function.This change correctly awaits the supervisor_debugger function which has been converted to an async function, maintaining the proper execution flow.
supervisor/misc/tasks.py (2)
227-227: Moved capture_exception to run in executor.The error capturing operation has been properly moved to run in an executor to prevent blocking the main event loop during I/O operations.
344-344: Moved capture_exception to run in executor.Similar to the other instance, this change properly moves the error capturing to run in an executor, consistently applying the pattern throughout the codebase.
supervisor/api/__init__.py (1)
415-415: Moved capture_exception to run in executor.The error capturing operation has been correctly moved to run in an executor, consistent with the pattern applied in other files and aligning with the PR's objective.
supervisor/resolution/check.py (1)
69-69: Moved exception capture to executorThe
capture_exceptioncall has been moved to run in an executor viaself.sys_run_in_executor. This is consistent with the PR objective of relocating potentially blocking I/O operations to executors.This change will prevent the exception capture process (which might involve network requests or file I/O) from blocking the main event loop, improving overall responsiveness of the application.
supervisor/plugins/multicast.py (1)
112-112: Moved exception capture to executorThe
capture_exceptioncall has been moved to run in an executor viaself.sys_run_in_executor. This is consistent with the PR objective of relocating potentially blocking I/O operations to executors.This change will prevent the exception capture process from blocking the main event loop, improving overall responsiveness of the application.
supervisor/resolution/notify.py (1)
39-39: Changed property access to async method callThe direct property access
self.sys_host.info.free_spacehas been changed to an asynchronous method callawait self.sys_host.info.free_space(). This indicates that retrieving free space information now happens asynchronously.This change is aligned with the PR's objective of moving potentially blocking operations (like disk space checks) to asynchronous execution. Since this is now an async call, it's correctly awaited within the f-string interpolation.
supervisor/utils/log_format.py (2)
3-3: Import looks good.
No issues with introducingasynciohere.
24-24: Asynchronous exception capture is well-implemented.
Runningcapture_exceptionin an executor helps avoid blocking the event loop.supervisor/homeassistant/core.py (4)
163-163: Async exception capture is consistent.
Usingself.sys_run_in_executorfor exception capture prevents blocking the event loop.
195-195: Good practice for capturing unexpected exceptions.
Continuing the installation in a loop while capturing any errors asynchronously is a sound approach.
560-560: Resilient error handling in watchdog.
Restart logic remains non-blocking by offloading Sentry capture to a separate thread.
572-572: Consistent error capture pattern.
Maintaining a uniform approach to exception handling across the watchdog restart improves supportability.supervisor/core.py (4)
172-172: Good improvement to non-blocking error handlingMoving the Sentry exception capture to run in an executor is a good practice that prevents blocking the async event loop during error processing.
189-189: Consistent async pattern for error handlingMoving error capture to an executor maintains consistent error handling approach throughout the system.
246-247: Correct async error capturingGood change to prevent blocking during HomeAssistant error processing.
251-251: Appropriate executor usageCorrectly moves error capturing to an executor, consistent with other error handling changes.
supervisor/os/data_disk.py (2)
340-341: Proper async error handlingMoving exception capture to an executor prevents blocking the event loop when errors occur during device formatting operations.
357-358: Consistent async patternThis change aligns with the project's move toward more consistent asynchronous error handling patterns.
supervisor/addons/addon.py (1)
1533-1533: Improved error handling in watchdog restartMoving the exception capture to an executor in the addon restart process aligns with the project's shift toward non-blocking error handling.
supervisor/backups/manager.py (2)
528-529: Enhanced backup process error handlingMoving exception capturing to an executor improves the responsiveness of the system during backup error scenarios.
721-722: Consistent async error handling during restoreThis change ensures restore operations don't block the event loop when capturing errors, consistent with other parts of the system.
supervisor/resolution/evaluate.py (1)
69-69: Good conversion to asynchronous error handling.The change to run capture_exception in an executor is a good improvement to avoid blocking the main event loop during error handling.
tests/mounts/test_manager.py (1)
6-6: Improving test stability with order-insensitive comparison.Using
unorderable_list_differencemakes the test more resilient to order changes in how mount operations are processed. This is a good practice when the order of operations doesn't matter, only that all expected operations are performed.Also applies to: 115-154
supervisor/plugins/base.py (1)
132-132: Good conversion to asynchronous error handling.The change to run capture_exception in an executor is a good improvement to avoid blocking the main event loop during error handling.
supervisor/host/apparmor.py (1)
74-76: Good conversion to asynchronous profile validation.Moving the AppArmor profile validation to run in an executor is a good improvement to avoid blocking the main event loop during potentially IO-intensive validation operations.
supervisor/api/addons.py (1)
215-215: Property access changed to async method call.The change from accessing
addon.long_descriptionas a property to awaiting it as an async method (await addon.long_description()) is correct since this is moving file I/O operations to an executor.supervisor/resolution/checks/free_space.py (2)
26-26: Property access changed to async method call.The modification to await
self.sys_host.info.free_space()is aligned with the PR objective to move disk operations to executor.
48-48: Property access changed to async method call.Similar to the previous change, awaiting the free space check is the correct approach.
supervisor/addons/manager.py (2)
173-173: Exception capture moved to executor.Moving the Sentry
capture_exceptionto an executor throughsys_run_in_executoris a good approach since it prevents blocking the main event loop during error handling.
391-391: Exception capture moved to executor.Consistent with the earlier change, this correctly moves the exception capture to an executor.
supervisor/api/middleware/security.py (1)
211-213: Error response function is now awaited.The change to await
api_return_erroris correct as the function has been made asynchronous as part of this PR.supervisor/docker/interface.py (2)
281-281: Changed error handling to use async executor pattern.This change moves the Sentry exception capture to run in an executor, ensuring error reporting doesn't block the main event loop.
397-397: Consistent async error handling implementation.Moving the exception capture to an executor keeps error reporting non-blocking and aligns with the pattern used throughout the codebase.
supervisor/utils/sentry.py (2)
46-49: Added important executor usage documentation.The updated docstring correctly clarifies that this function must be called within an executor context, which matches the implementation changes made throughout the codebase.
57-60: Added critical executor requirement to docstring.This documentation update is essential since all calls to this function are being migrated to run within executors throughout the codebase.
supervisor/plugins/manager.py (3)
83-83: Updated exception handling to run in executor.This change ensures that Sentry exception capture runs in a separate executor context rather than potentially blocking the main event loop.
116-116: Consistent use of asynchronous error handling.Moving the exception capture to an executor maintains consistency with the pattern applied throughout the codebase for non-blocking error handling.
134-134: Improved error handling in shutdown sequence.Converting exception capture to run in an executor enhances shutdown reliability by preventing the main event loop from being blocked during error reporting.
supervisor/docker/addon.py (2)
609-609: Updated DNS error handling to use async pattern.This change makes the error reporting non-blocking by moving the Sentry capture to an executor, following the same pattern used throughout the codebase.
790-790: Consistent async error handling during container stop.Moving the exception capture to an executor during DNS host deletion ensures error reporting doesn't block the container stop process.
supervisor/addons/model.py (1)
637-650:❓ Verification inconclusive
Good asynchronous refactoring of
long_descriptionmethod.The transformation from a synchronous property to an asynchronous method is a well-structured change. Moving file I/O operations to run in an executor prevents blocking the event loop during potentially slow disk operations.
Make sure all callers of this method have been updated to use
awaitwhen accessing this method, as it has changed from a property to an async method.
🏁 Script executed:
#!/bin/bash # Find all instances where long_description is accessed to ensure proper awaiting rg "\.long_description[^(]" --type pyLength of output: 39
Asynchronous refactoring approved, but please manually verify usage.
The refactoring convertinglong_descriptionfrom a synchronous property to an asynchronous method is well done—offloading file I/O to an executor avoids blocking the event loop. However, since our automated search for callers not usingawait(via the providedrgcommand) did not return any output, it's unclear if all usages have been updated correctly. Please confirm that every invocation usesawait, for example:
await instance.long_description()instead ofinstance.long_descriptionsupervisor/dbus/network/__init__.py (3)
3-3: Added required asyncio import.Required for the asynchronous error handling below.
227-229: Good practice: Moving error capture to executor.Moving
capture_exceptionto run in an executor prevents blocking the event loop during error handling operations.
235-237: Consistent error handling approach.This change matches the error handling pattern applied above, ensuring consistent non-blocking error handling throughout the method.
supervisor/api/host.py (1)
101-104: Updated to use async disk space methods.The code has been properly updated to await the disk space information methods, which have been changed from synchronous properties to asynchronous methods elsewhere in the codebase.
This change ensures disk I/O operations don't block the event loop, improving system responsiveness.
supervisor/jobs/decorator.py (1)
316-316: Good practice: Moving error capture to executor.Moving the
capture_exceptioncall to run in an executor prevents blocking the event loop during error handling operations.supervisor/bootstrap.py (1)
299-304: Convertedsupervisor_debuggerto async function.The function has been updated to be asynchronous and now imports the debugpy module using an executor. This change aligns with the PR objective of moving potentially blocking operations off the main thread.
supervisor/os/manager.py (2)
219-228: File I/O operations moved to an executor.The file operations (open, write, close) have been properly moved to run in an executor to prevent blocking the main thread. Good job adding the try/finally block to ensure the file is always closed, even if an exception occurs during writing.
388-388: Exception handling moved to an executor.The capture_exception call is now executed in an executor, preventing potential blocking during error handling. This is consistent with the PR's objective of moving synchronous calls to executors.
supervisor/api/utils.py (4)
131-143: Madeapi_return_errorfunction asynchronous.The function has been properly converted to async, and the
format_messagecall is now correctly awaited. This change is necessary sinceformat_messagehas been made asynchronous as part of the effort to move text reading operations to executors.
66-70: Updated calls to await the now asynchronousapi_return_errorfunction.All calls to
api_return_errorin the exception handling blocks are now properly awaited, which is necessary since the function has been made asynchronous.
79-79: Added await toapi_return_errorcall.This call has been properly updated to await the now asynchronous function.
110-119: Updatedapi_return_errorcalls to use await.All calls within the error handling blocks have been properly updated to use await for the now asynchronous
api_return_errorfunction.tests/utils/test_log_format.py (3)
6-12: Updated test to be asynchronous.The test function has been properly converted to async and now awaits the
format_messagecall, which is necessary since the function being tested has been made asynchronous.
15-21: Converted test to async pattern.This test function has been correctly updated to use async/await syntax to match the changes in the
format_messagefunction.
24-27: Updated exception test to use async pattern.The exception test has been properly converted to be asynchronous and now awaits the
format_messagecall.supervisor/api/store.py (7)
5-5: No concerns on new import.
This import is necessary for the new_read_static_filehelper and does not present any issues.
112-115: Asynchronous conversion looks good.
Switching_generate_addon_informationto an async function is consistent with the rest of the changes.
159-159: Asynchronous property retrieval is appropriate.
Awaitingaddon.long_description()aligns with the new asynchronous flow.
241-241: No issues with awaiting add-on information.
This call is consistent with the new async flow and ensures extended information is retrieved.
250-250: Reading binary file in text mode.
This call expects bytes but_read_static_filecurrently uses"r". Please see the previously suggested fix on lines 72-79.
259-259: Reading binary file in text mode.
This call expects bytes but_read_static_filecurrently uses"r". Refer to the fix proposed in_read_static_file.
273-273: Reading text is fine, but verify.
addons_addon_changelogexpects a string, aligning better with"r"mode. If the add-on changelog is purely text, there is no issue. Otherwise, consider a binary read.supervisor/store/repository.py (2)
77-80: Docstring clarification is helpful.
Indicating thatvalidatemust be run in an executor clarifies the synchronous nature of the I/O involved.
110-110: Running validation in executor is consistent.
Invokingvalidatewithinsys_run_in_executoraligns with the updated docstring requirements and ensures non-blocking behavior.supervisor/hardware/disk.py (4)
52-55: Docstring improvement recognized.
Explicitly stating thatget_disk_total_spacemust run in an executor clarifies when to perform blocking I/O.
60-63: Executor requirement is clear.
Similarly, clarifyingget_disk_used_spaceusage in an executor helps avoid blocking the main thread.
68-71: No problems with added docstring mention.
Theget_disk_free_spacemethod follows the same pattern of requiring executor usage.
125-128: Documentation clarity is beneficial.
Stating thatget_disk_life_timerequires executor usage will help avoid performance bottlenecks in asynchronous code.supervisor/mounts/mount.py (5)
211-211: Good async pattern for error handling.Moving the error capture operation to an executor prevents blocking the main event loop when sending error reports, which aligns with the PR objective.
224-224: Good async pattern for error handling.Similar to the previous change, this moves error capturing to an executor to prevent blocking the main event loop.
282-299: Good refactoring and async pattern implementation.This change improves code organization by extracting the folder validation logic into a standalone function that's executed in a background thread. The error messages are clear and descriptive, and the implementation properly handles various edge cases.
496-505: Good refactoring and async pattern for credential handling.The credential writing logic has been appropriately refactored into a dedicated function and executed in a background thread, which prevents blocking the main event loop during file I/O operations.
511-511: Good async pattern for file operations.Moving the file deletion operation to an executor prevents blocking the main event loop, consistent with the pattern established in the rest of this file.
supervisor/host/info.py (5)
106-109: Good addition of virtualization property.This property provides useful information about the virtualization environment, which can be valuable for diagnostics and system understanding.
110-115: Good conversion to async method for disk operations.Converting this property to an async method that runs disk space calculation in a background thread prevents blocking the main event loop during I/O operations.
117-122: Good conversion to async method for disk operations.Similar to the previous change, this properly moves potentially blocking disk operations to a background thread.
124-129: Good conversion to async method for disk operations.Consistent with the pattern, this change properly handles disk operations asynchronously.
131-136: Good conversion to async method for disk operations.This change follows the established pattern of moving potentially blocking disk operations to a background thread, improving overall system responsiveness.
mdegat01
force-pushed
the
read-text-to-executor
branch
from
1eaa22e to
d3a0cf4
Compare
supervisor/addons/addon.py
Outdated
| @@ -1530,7 +1530,7 @@ async def _restart_after_problem(self, state: ContainerState): | |||
| except AddonsError as err: | |||
| attempts = attempts + 1 | |||
| _LOGGER.error("Watchdog restart of addon %s failed!", self.name) | |||
| capture_exception(err) | |||
| await self.sys_run_in_executor(capture_exception, err) | |||
There was a problem hiding this comment.
I was actually wondering that, is capture_exception really blocking I/O? I guess it is, but it nowhere really says, from what I can tell? 🤔
In any case, I am a bit worried that we don't get some very fatal issues anymore with this e.g. what if the error leads to a segfault right afterwards? In that case the Sentry event might not make it out.
Now I am not sure, maybe that is a bit scripted, and given that people who opt-in to sentry might actually block unnecessarily in their production system, we should probably really do this in an executor.
But I'd like to have this centralized. Can you create a async_capture_exception which calls the executor? This would allow us to "opt-out" (call synchronously), e.g. for a dev instance or something.
There was a problem hiding this comment.
I was actually wondering that, is capture_exception really blocking I/O? I guess it is, but it nowhere really says, from what I can tell? 🤔
It is. Though not necessarily because of Sentry. I also am not sure if the library i blocking I/O or not. But it doesn't seem to support async and makes HTTP calls so I think it is?
But the reason I know its I/O is because of us. We add read the remaining free space from the disk every time and attach that info as metadata in the call here:
supervisor/supervisor/misc/filter.py
Line 83 in 696dcf6
That was how it ended up in this PR. I found the read_text call in the free_space property and backtracked to where it was used.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
supervisor/addons/addon.py(2 hunks)supervisor/addons/manager.py(3 hunks)supervisor/api/__init__.py(2 hunks)supervisor/backups/manager.py(3 hunks)supervisor/core.py(4 hunks)supervisor/dbus/network/__init__.py(2 hunks)supervisor/docker/addon.py(3 hunks)supervisor/docker/interface.py(3 hunks)supervisor/homeassistant/core.py(5 hunks)supervisor/jobs/decorator.py(3 hunks)supervisor/misc/tasks.py(3 hunks)supervisor/mounts/manager.py(2 hunks)supervisor/mounts/mount.py(3 hunks)supervisor/os/data_disk.py(3 hunks)supervisor/os/manager.py(2 hunks)supervisor/plugins/audio.py(2 hunks)supervisor/plugins/base.py(2 hunks)supervisor/plugins/cli.py(2 hunks)supervisor/plugins/dns.py(2 hunks)supervisor/plugins/manager.py(4 hunks)supervisor/plugins/multicast.py(2 hunks)supervisor/plugins/observer.py(2 hunks)supervisor/resolution/check.py(2 hunks)supervisor/resolution/checks/dns_server.py(2 hunks)supervisor/resolution/checks/dns_server_ipv6.py(2 hunks)supervisor/resolution/evaluate.py(2 hunks)supervisor/resolution/fixup.py(2 hunks)supervisor/supervisor.py(2 hunks)supervisor/utils/dbus.py(2 hunks)supervisor/utils/log_format.py(2 hunks)supervisor/utils/sentry.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (30)
- supervisor/os/manager.py
- supervisor/plugins/cli.py
- supervisor/core.py
- supervisor/plugins/audio.py
- supervisor/misc/tasks.py
- supervisor/resolution/checks/dns_server.py
- supervisor/resolution/evaluate.py
- supervisor/plugins/observer.py
- supervisor/plugins/multicast.py
- supervisor/resolution/checks/dns_server_ipv6.py
- supervisor/api/init.py
- supervisor/utils/dbus.py
- supervisor/backups/manager.py
- supervisor/plugins/manager.py
- supervisor/dbus/network/init.py
- supervisor/addons/manager.py
- supervisor/plugins/base.py
- supervisor/resolution/check.py
- supervisor/addons/addon.py
- supervisor/docker/interface.py
- supervisor/resolution/fixup.py
- supervisor/utils/log_format.py
- supervisor/homeassistant/core.py
- supervisor/plugins/dns.py
- supervisor/jobs/decorator.py
- supervisor/os/data_disk.py
- supervisor/mounts/mount.py
- supervisor/mounts/manager.py
- supervisor/docker/addon.py
- supervisor/supervisor.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
🔇 Additional comments (4)
supervisor/utils/sentry.py (4)
3-3: No concerns on this new import.
50-53: Docstring update is consistent with the transition to an executor-based call.
60-67: Asynchronous event capture logic looks good.
71-74: Docstring clarification aligns with executor usage.
mdegat01
force-pushed
the
read-text-to-executor
branch
from
b1e4440 to
aa2f482
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/api/test_supervisor.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.13.2
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
mdegat01
force-pushed
the
read-text-to-executor
branch
from
b47b65f to
ed3f50f
Compare
mdegat01
force-pushed
the
read-text-to-executor
branch
from
ed3f50f to
baca69f
Compare
Proposed change
Begin moving calls to
read_textto executor. This PR was intended to cover all calls toread_textbut then I realized our sentry methodcapture_exceptionwas doing a call toread_text. That required a lot of changes to move all calls to that to the executor so I stopped there and will continue in a follow-up PR.Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit