Revamp context managers, part 1 #302
Conversation
WalkthroughThis pull request refactors multiple network adapters and related client adapters by converting class-based implementations into asynchronous context manager functions. The documentation is updated accordingly, replacing Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as Adapter Function
participant L as TemporaryTcpListener
C->>A: Invoke adapter (e.g., FabricAdapter, NovncAdapter)
A->>L: Enter async context to initialize connection
L-->>A: Provide connection details
A-->>C: Yield connection object for usage
C->>A: Operate with connection
A->>L: Exit context and cleanup resources
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
❌ Deploy Preview for jumpstarter-docs failed. Why did it fail? →
|
|
Followup to #301, ensures that clients are properly closed on sys.exit. |
NickCao
force-pushed
the
context-manager-revamp
branch
from
6061bf4 to
a55e0a8
Compare
NickCao
changed the title
![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 (5)
packages/jumpstarter/jumpstarter/client/adapters.py (1)
5-12: Prevent potential KeyError if'client'is missing inkwargs.
Currently, this code expects'client'to exist, which may lead to a runtime error if it’s not provided. Consider adding a graceful check or documentation to mitigate the risk:def wrapper(*args, **kwargs): + client = kwargs.get("client") + if not client: + raise ValueError("'client' is missing in kwargs") with client.portal.wrap_async_context_manager(f(*args, **kwargs)) as res: yield respackages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
12-21: Consider adding error handling for socket connection.
Ifsock.connect(addr)fails or times out, the async block will raise an exception. Adding a try-except block could improve resilience:async def AsyncPexpectAdapter(*, client: DriverClient, method: str = "connect"): async with AsyncTcpPortforwardAdapter(client=client, method=method) as addr: sock = await run_sync(socket.socket, socket.AF_INET, socket.SOCK_STREAM) - await run_sync(sock.connect, addr) + try: + await run_sync(sock.connect, addr) + except OSError as err: + await run_sync(sock.close) + raise RuntimeError(f"Failed to connect to {addr}") from err try: yield fdspawn(sock) finally: await run_sync(sock.close)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
10-14: Consider adding robust error/log handling.
While thehandlerfunction straightforwardly forwards data betweenconnandstream, it might be beneficial to log errors or exceptions to improve debugging and avoid silent failures if either side closes unexpectedly.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py (1)
14-19: Inline handler function is effective.
The nestedhandlerneatly ties together the connection and the client stream. It might be beneficial to add minimal logging or exception handling for improved observability.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py (1)
43-45: Consider enhancing error handling during close.While using
suppress(Error)is good for preventing cascading errors, consider logging suppressed errors to aid in debugging.async def aclose(self): + from logging import getLogger + logger = getLogger(__name__) with suppress(Error): - await self.file.close() + try: + await self.file.close() + except Error as e: + logger.debug(f"Suppressed error while closing file: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/source/api-reference/adapters/network.md(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py(3 hunks)packages/jumpstarter/jumpstarter/client/adapters.py(1 hunks)packages/jumpstarter/jumpstarter/common/tempfile.py(1 hunks)packages/jumpstarter/jumpstarter/common/utils.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
🔇 Additional comments (17)
packages/jumpstarter/jumpstarter/client/adapters.py (1)
1-2: Imports look appropriate for decorator-based context management.
No issues are observed. The usage ofcontextlibandfunctoolshelps keep this implementation concise.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (2)
2-2: Well-structured imports for asynchronous operation.
The introduction ofasynccontextmanager,run_sync, andblockingis aligned with an asynchronous design that ensures proper resource usage.Also applies to: 4-4, 7-9
24-24: Appropriate use ofblockingfor synchronous compatibility.
CreatingPexpectAdapterfromAsyncPexpectAdapterin a blocking manner ensures broad usability.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py (3)
1-2: Imports align well with partial application and async management.
No concerns here. This addition is clean and suits the new context-based pattern.
8-11: Imports for adapter components are coherent.
These references from local modules andjumpstarter.client.adaptersintegrate smoothly with the updated context manager design.
14-38: Functional approach toFabricAdapteris clear and consistent.
The shift from a class-based to a function-based async context manager aligns with the overall PR objective. The code is readable and logically structured.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (5)
1-2: Imports look good.
No issues identified with these newly introduced imports.
17-30: Async context manager usage is well-structured.
Switching to@asynccontextmanageralong withTemporaryTcpListenereffectively ensures that resources are cleaned up once the context exits. This is a neat improvement over manually handling enter/exit calls.
33-33: Blocking wrapper usage looks appropriate.
WrappingAsyncTcpPortforwardAdapterwithblockingis consistent with the new async-to-sync approach. Ensure that this pattern is thoroughly tested in synchronous workflows.
36-43: Parallel design for Unix port forwarding.
Similar strategy toAsyncTcpPortforwardAdapter, ensuring consistent usage of async context management for Unix sockets.
46-46: Unix blocking adapter is aligned with TCP approach.
This maintains uniformity across TCP and Unix domain sockets.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py (2)
11-13: Conversion to async context manager is clear.
DecoratingNovncAdapterwith@blockingand@asynccontextmanagersimplifies setup/teardown logic. Good improvement over the older class-based pattern.
21-22: Yielding noVNC URL is a user-friendly design.
Returning a direct noVNC URL with the ephemeral TCP port is convenient. Verify if HTTPS on “novnc.com” is suitable for production usage or if a custom host is needed.packages/jumpstarter/jumpstarter/common/tempfile.py (1)
30-30: Revisiting host and port reuse defaults.
Switchinglocal_hostto"127.0.0.1"is a good security measure by restricting external access. Enablingreuse_port=Truecan be helpful for certain parallel socket usage, but verify that it won’t introduce unintended port-sharing scenarios in your environment.packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py (1)
48-69: LGTM! Clean conversion to async context manager.The refactoring from class-based to function-based implementation with proper async context management is well done. The logic for handling presigned URLs and file streaming is preserved while improving resource management.
packages/jumpstarter/jumpstarter/common/utils.py (1)
21-25: LGTM! Robust error handling for client closure.The consistent implementation of try-finally blocks across all context managers ensures proper client cleanup, even in error scenarios. This aligns perfectly with the PR's objective of proper resource management.
Also applies to: 32-36, 55-59
docs/source/api-reference/adapters/network.md (1)
6-7: LGTM! Documentation accurately reflects implementation changes.The documentation has been properly updated to reflect the transition from class-based to function-based implementations while maintaining clear examples.
Also applies to: 10-11, 14-15, 18-19, 22-23
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/client/client.py (1)
53-53: NestingExitStackinstack=stack.enter_context(ExitStack()).While this is valid, the reuse of the variable name
stackmight be slightly confusing. Consider renaming the nested stack to improve clarity.- stack=stack.enter_context(ExitStack()), + nested_stack = stack.enter_context(ExitStack()),packages/jumpstarter/jumpstarter/common/utils.py (1)
28-38: Consider consolidating duplicate cleanup logic.The client cleanup logic is duplicated between
serve_asyncandserve. Consider extracting it into a helper function.+def _cleanup_client(client): + if hasattr(client, "close"): + client.close() + @contextmanager def serve(root_device: Driver): with start_blocking_portal() as portal: with ExitStack() as stack: with portal.wrap_async_context_manager(serve_async(root_device, portal, stack)) as client: try: yield client finally: - if hasattr(client, "close"): - client.close() + _cleanup_client(client)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/base.py(3 hunks)packages/jumpstarter/jumpstarter/client/client.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(3 hunks)packages/jumpstarter/jumpstarter/common/utils.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(6 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(2 hunks)packages/jumpstarter/jumpstarter/listener_test.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (23)
packages/jumpstarter/jumpstarter/config/client.py (7)
2-2: ImportingExitStackfor improved context management.This import addition looks appropriate for handling complex resource cleanup with multiple context managers.
65-69: NestedExitStackusage withinleasefunction.Introducing
ExitStackhere is a good step towards comprehensive resource management. This ensures all acquired resources are properly cleaned up if an exception occurs.
73-74: ConsistentExitStackusage inrequest_lease.Reusing the same pattern as in
leasehelps maintain uniform resource handling across both methods.
84-89: Newrequest_lease_asyncincludes astackparameter.Passing
ExitStackinto the asynchronous method centralizes resource cleanup, improving reliability should exceptions occur. Great approach.
98-98: ProvidingstacktoLease(...).Successfully integrating
ExitStackintoLeaseensures advanced resource lifecycle management for the leased resources.
117-119: Refinedlease_asyncsignature withExitStack.Accepting
stackmirrors the synchronous approach, giving you complete control over the context's lifetime in an async environment.
132-132: Injectingstack=stackinto theLeasecontext.Maintaining consistency by reusing the same
stackreference ensures the correct nesting of contexts for resource cleanup.packages/jumpstarter/jumpstarter/client/client.py (4)
2-2: AddedExitStackimport.This addition complements the asynchronous context usage of
asynccontextmanagerfor better resource handling.
16-16: Revisedclient_from_pathsignature to acceptstack.Including the
ExitStackparameter is an excellent way to unify resource management between synchronous and asynchronous contexts.
20-20: Yielding fromclient_from_channelwithstack.Passing
stackseamlessly fromclient_from_pathensures a cohesive context lifecycle for channels.
26-26: Extendedclient_from_channelto acceptstack.Exposing
stackhere allows downstream usage of nested contexts while creating clients.packages/jumpstarter/jumpstarter/client/base.py (4)
7-7: IncorporatedExitStackimport.Good step toward consolidating resource finalization under a single exit strategy.
34-34: Addedstack: ExitStackfield toDriverClient.Storing
ExitStackinsideDriverClientcentralizes cleanup logic, minimizing the risk of resource leaks.
90-90: Usingself.stack.enter_contextinopen_stream.Leveraging the stored
ExitStackfor streaming clarifies ownership of the stream resource and ensures consistent teardown.
96-96: Closing theExitStackinclose.This definitively finalizes any pending contexts opened via
open_stream. Double-check that no active tasks will inadvertently get closed.packages/jumpstarter/jumpstarter/common/utils.py (2)
15-26: LGTM! Robust resource cleanup implementation.The addition of
ExitStackand the try-finally block ensures proper cleanup of client resources, even in case of exceptions. This aligns well with the PR's objective of ensuring proper client closure on system exit.
40-61: LGTM! Consistent error handling pattern.The env_async function follows the same robust error handling pattern as serve_async, ensuring consistent resource cleanup across the codebase.
packages/jumpstarter/jumpstarter/listener_test.py (2)
43-57: LGTM! Proper resource management in tests.The test correctly uses ExitStack to manage resources, ensuring proper cleanup even in test scenarios.
94-115: LGTM! Comprehensive concurrent connection testing.The test effectively verifies concurrent connections while maintaining proper resource management through ExitStack.
packages/jumpstarter/jumpstarter/config/exporter_test.py (1)
54-65: LGTM! Proper resource management in exporter test.The test correctly integrates ExitStack for resource management in the lease_async call.
packages/jumpstarter/jumpstarter/client/lease.py (2)
28-41: LGTM! Proper integration of ExitStack in Lease class.The addition of the required
stackfield ensures proper resource management throughout the lease lifecycle.
156-160: LGTM! Consistent resource management in connect_async.The connect_async method correctly uses the stack parameter for client creation, maintaining consistent resource management.
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py (1)
25-25:Details
❓ Verification inconclusive
LGTM! Improved context management implementation.
The change to use
self.stack.enter_context()is a more robust approach that ensures proper cleanup of resources, especially during system exit. This aligns well with the PR objectives.Let's verify the implementation of
self.stack:
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of DriverClient's stack attribute ast-grep --pattern $'class DriverClient { $$$ def __init__($$$) { $$$ self.stack = $$$ $$$ } $$$ }'Length of output: 120
Below are additional shell script commands to further verify that the
self.stackattribute is correctly initialized and that thepexpect()method is implemented properly as a context manager. Once you run these scripts and confirm that both are in place as expected, you can finalize the review.
ACTION: Verify and Finalize Context Management Implementation
The change to use
self.stack.enter_context(self.pexpect())is a more robust approach that ensures proper resource cleanup—especially during system exit—aligning with the PR objectives. However, please confirm that:• The
self.stackattribute is properly initialized (typically in the constructor of the client class).
• Thepexpect()method is implemented as a valid context manager.Once these details are verified, the refactoring can be considered complete.
🏁 Script executed:
#!/bin/bash # Verify the assignment of self.stack in the client file rg "self\.stack\s*=" -N packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.pyLength of output: 100
🏁 Script executed:
#!/bin/bash # Verify the implementation (definition) of the pexpect method in the client file rg "def pexpect\(" -N packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.pyLength of output: 121
Attention: Context Management Verification Reminder
The refactoring of the context management—using
self.stack.enter_context(self.pexpect())—is a strong improvement ensuring automatic resource cleanup. We confirmed that thepexpect()method is defined as a context manager. However, our search did not reveal an assignment for theself.stackattribute in the file. Please verify that:
- Initialization of
self.stack: Ensure thatself.stackis initialized (typically with an instance ofExitStack) in the constructor or an inherited initializer.- Context Consistency: Confirm that the lifecycle of
self.stackis correctly managed so that resource cleanup occurs appropriately.Once the initialization of
self.stackis verified or corrected, the implementation looks solid in meeting the PR’s objectives.
NickCao
force-pushed
the
context-manager-revamp
branch
from
0bdb394 to
9ba37c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter/jumpstarter/client/client.py (1)
16-16: LGTM! Robust resource management implementation.The changes effectively ensure proper cleanup of client resources by:
- Passing the ExitStack through the client creation chain
- Creating nested ExitStacks for each client in the hierarchy
This implementation guarantees that resources are released in the correct order when the system exits.
Consider adding a comment explaining the purpose of
stack.enter_context(ExitStack())on line 53, as it creates a new nested ExitStack for each client:client = client_class( uuid=UUID(uuid), labels=report.labels, channel=channel, portal=portal, + # Create a new ExitStack for this client to manage its resources independently stack=stack.enter_context(ExitStack()), children={reports[k].labels["jumpstarter.dev/name"]: clients[k] for k in topo[uuid]}, )Also applies to: 20-20, 26-26, 53-53
packages/jumpstarter/jumpstarter/client/base.py (1)
34-34: Add documentation for the stack field.The
stackfield should be documented to explain its purpose in managing client resources.portal: BlockingPortal - stack: ExitStack + stack: ExitStack + """ExitStack for managing client resources and ensuring proper cleanup on exit"""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/base.py(3 hunks)packages/jumpstarter/jumpstarter/client/client.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(2 hunks)packages/jumpstarter/jumpstarter/common/utils.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(2 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(2 hunks)packages/jumpstarter/jumpstarter/listener_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/jumpstarter/jumpstarter/config/exporter_test.py
- packages/jumpstarter/jumpstarter/client/lease.py
- packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
- packages/jumpstarter/jumpstarter/common/utils.py
- packages/jumpstarter/jumpstarter/listener_test.py
- packages/jumpstarter/jumpstarter/config/client.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (4)
packages/jumpstarter/jumpstarter/client/client.py (1)
2-2: LGTM! Good choice using ExitStack.The addition of
ExitStackis a good choice for managing multiple context managers dynamically, which aligns well with the PR's objective of ensuring proper client closure.packages/jumpstarter/jumpstarter/client/base.py (3)
7-7: LGTM! Consistent import organization.The ExitStack import is correctly placed alongside other context management imports.
90-90: LGTM! Proper resource management.Using
stack.enter_contextensures that stream resources are properly managed and will be cleaned up when the client is closed.
96-96: LGTM! Comprehensive cleanup implementation.Using
stack.close()ensures all resources managed by the stack are properly cleaned up, and the integration with__del__provides a safety net for cleanup.
NickCao
force-pushed
the
context-manager-revamp
branch
from
9ba37c5 to
82cc95e
Compare
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py
Show resolved
Hide resolved
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Looks good, a nice improvement.
I wonder if there would be a way of getting to the "next mile", doing it at the base client :D
fr>>> from jumpstarter.common.utils import serve
>>> client = serve(PySerial(url="loop://"))
>>> cl = client.open()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: '_GeneratorContextManager' object has no attribute 'open'
>>> cl = client.__enter__()
>>> p = cl.open()
>>> dir(p)
|
oops it fails now, I guess it doesn,t work on top of the dbus without adapting the dbus now. |
Indeed, fixing in a second. |
NickCao
force-pushed
the
context-manager-revamp
branch
from
3c5b186 to
83d06fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
packages/jumpstarter/jumpstarter/client/adapters.py (1)
5-12: Add docstring to explain the decorator's purpose.The implementation looks good and follows best practices by using
@wrapsand@contextmanager. However, adding a docstring would help users understand its purpose.def blocking(f): + """ + Decorator that converts an async context manager into a blocking one. + + Args: + f: An async context manager function + Returns: + A synchronous context manager function + """ @wraps(f) @contextmanager def wrapper(*args, **kwargs):packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py (1)
10-20: Consider enhancing error handling for socket operations.The implementation correctly manages socket resources with proper cleanup. However, socket operations could benefit from more robust error handling.
@contextmanager def PexpectAdapter(*, client: DriverClient, method: str = "connect"): with TcpPortforwardAdapter(client=client, method=method) as addr: - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock.connect(addr) + sock = None + try: + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.connect(addr) + except socket.error as e: + if sock: + sock.close() + raise ConnectionError(f"Failed to connect to {addr}: {e}") from e try: yield fdspawn(sock) finally: - sock.close() + if sock: + sock.close()packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py (1)
11-32: Add docstring and enhance error handling for stream operations.The implementation correctly manages async resources and stream forwarding. However, it would benefit from documentation and more robust error handling.
@blocking @asynccontextmanager async def NovncAdapter(*, client: DriverClient, method: str = "connect"): + """ + Creates a NoVNC connection adapter. + + Args: + client: The driver client + method: The connection method to use (default: "connect") + + Returns: + A NoVNC URL for connecting to the remote display + """ async def handler(conn): async with conn: - async with client.stream_async(method) as stream: - async with WebsocketServerStream(stream=stream) as stream: - async with forward_stream(conn, stream): - pass + try: + async with client.stream_async(method) as stream: + async with WebsocketServerStream(stream=stream) as stream: + async with forward_stream(conn, stream): + pass + except Exception as e: + # Log error before re-raising to aid debugging + import logging + logging.error(f"Stream forwarding failed: {e}") + raisepackages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py (1)
48-69: Extract magic numbers and enhance error handling for presigned URLs.The implementation is solid but would benefit from extracting constants and adding error handling for presigned URL generation.
+# Constants for configuration +CHUNK_SIZE = 65536 # 64KB chunks for file reading +PRESIGN_EXPIRY_SECONDS = 60 + @blocking @asynccontextmanager async def OpendalAdapter( *, client: DriverClient, operator: Operator, # opendal.Operator for the storage backend path: str, # file path in storage backend relative to the storage root mode: Literal["rb", "wb"] = "rb", # binary read or binary write mode ): # if the access mode is binary read, and the storage backend supports presigned read requests if mode == "rb" and operator.capability().presign_read: - # create presigned url for the specified file with a 60 second expiration - presigned = await operator.to_async_operator().presign_read(path, expire_second=60) - yield PresignedRequestResource( - headers=presigned.headers, url=presigned.url, method=presigned.method - ).model_dump(mode="json") + try: + # create presigned url for the specified file + presigned = await operator.to_async_operator().presign_read( + path, expire_second=PRESIGN_EXPIRY_SECONDS + ) + yield PresignedRequestResource( + headers=presigned.headers, url=presigned.url, method=presigned.method + ).model_dump(mode="json") + except Error as e: + raise ValueError(f"Failed to generate presigned URL for {path}: {e}") from epackages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py (2)
14-16: Stacked decorators may require careful testing
Using both@blockingand@asynccontextmanageris a valid approach to expose an async context manager via a synchronous interface. However, verify that concurrency scenarios and error propagation behave as expected with this layering.
27-37: Consider enhanced error handling
The code correctly yields a FabricConnection, but ifTemporaryTcpListeneror the connection setup fails, there is no fallback or retry. Adding try-except blocks or clearer error reporting might improve reliability.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/dbus.py (1)
8-31: Global environment variable mutation may affect concurrency
Changing environment variables in a context manager is fine for single-threaded usage but can lead to race conditions if multiple tasks runDbusAdapterconcurrently. Consider a more isolated approach (e.g., spawning separate processes or providing per-invocation configuration) to avoid collisions.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (2)
10-15: Handler function clarity
Thehandlerfunction clearly coordinates the connection and stream forwarding. For extra robustness, consider logging exceptions or transient errors within this block if remote endpoints disconnect unexpectedly.
17-32: TCP port-forward adapter logic
UsingTemporaryTcpListenerwith an async context manager is clean. Watch out for ephemeral port collisions or lengthy connection times if many adapters spin up in parallel. Adding optional parameters (e.g., backlog size) or improved logging could aid debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
docs/source/api-reference/adapters/network.md(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/dbus.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/novnc.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/pexpect.py(1 hunks)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py(1 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py(3 hunks)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py(1 hunks)packages/jumpstarter/jumpstarter/client/adapters.py(1 hunks)packages/jumpstarter/jumpstarter/client/base.py(3 hunks)packages/jumpstarter/jumpstarter/client/client.py(3 hunks)packages/jumpstarter/jumpstarter/client/lease.py(2 hunks)packages/jumpstarter/jumpstarter/common/tempfile.py(1 hunks)packages/jumpstarter/jumpstarter/common/utils.py(4 hunks)packages/jumpstarter/jumpstarter/config/client.py(2 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(2 hunks)packages/jumpstarter/jumpstarter/listener_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/jumpstarter/jumpstarter/client/base.py
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/config/exporter_test.py
- packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/client.py
- packages/jumpstarter/jumpstarter/listener_test.py
- packages/jumpstarter/jumpstarter/client/lease.py
- packages/jumpstarter/jumpstarter/common/tempfile.py
- packages/jumpstarter/jumpstarter/common/utils.py
- packages/jumpstarter/jumpstarter/client/client.py
- docs/source/api-reference/adapters/network.md
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (7)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/adapter.py (1)
15-46: LGTM! Well-structured stream wrapper with proper error handling.The AsyncFileStream implementation correctly wraps opendal.AsyncFile with proper error handling and stream compatibility.
packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/fabric.py (3)
1-2: Imports look good
No issues found in these import statements; usage ofasynccontextmanagerandpartialappears appropriate for this context.
8-10: Imports for handler, DriverClient, and blocking
These newly introduced imports align well with the intended functionality of forwarding connections and providing a blocking interface for an async context.
17-17: Existing comment about enforcing keyword-only arguments
Mangelajo’s past remark remains relevant: using*ensures all parameters are passed as keywords, which can improve readability but may limit future flexibility.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/dbus.py (1)
1-5: Initial imports
The switch from a class-based to a function-based adapter makes sense, and these imports appear consistent with that transition.packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (2)
1-5: Revised imports
The changes here align with the new async function-based adapters. No issues detected.
34-42: Unix domain socket forwarding
This implementation follows the same pattern as TCP forwarding and looks consistent. The approach appears sound for local IPC. Ensure that permissions and file cleanup in the temp directory are carefully handled downstream.
This would bring us back to the "hacky" realm, there has to be at least a toplevel context manager managing the ExitStack. |
Summary by CodeRabbit
Documentation
Refactor
ExitStack.