RSDK-10220: Create resource clients on-demand without server check#525
RSDK-10220: Create resource clients on-demand without server check#525claude[bot] wants to merge 2 commits intomainfrom
Conversation
RobotClient.getResource() now creates resource clients on-demand using the Registry when they are not found in the ResourceManager, instead of throwing ResourceNotFoundException. This avoids blocking client creation on server availability, offloading error handling to the consumer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove unused `import 'package:viam_sdk/src/components/sensor/sensor.dart'` - Replace `expect(() => client.position(), throwsA(anything))` with `await expectLater(client.position(), throwsA(anything))` to properly await the async gRPC failure in the last test case Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return _manager.getResource<T>(name); | ||
| try { | ||
| return _manager.getResource<T>(name); | ||
| } on ResourceNotFoundException { |
There was a problem hiding this comment.
lookupSubtype throws a generic Exception (message: "Subtype not registered in registry") when the subtype is unknown — not a ResourceNotFoundException. This changes the error contract: callers who catch ResourceNotFoundException to handle "resource not available" will miss the case where the subtype is unregistered. Consider re-throwing as ResourceNotFoundException, or documenting clearly that an unregistered subtype propagates a raw Exception.
| final registration = Registry.instance.lookupSubtype(Subtype.fromResourceName(name)); | ||
| final resource = registration.rpcClientCreator(name.name, _channel); | ||
| _manager.register(name, resource); | ||
| return resource as T; |
There was a problem hiding this comment.
resource as T is an unchecked cast at runtime. If the caller passes a mismatched type (e.g. getResource<Camera>(motorName)), this throws a TypeError here rather than at _manager.getResource<T>. This is consistent with the manager's own cast, but worth noting: there is no guard if T does not extend Resource. This is the same exposure that already existed in the manager path, so no change required — just confirming it is intentional.
| return _manager.getResource<T>(name); | ||
| } on ResourceNotFoundException { | ||
| final registration = Registry.instance.lookupSubtype(Subtype.fromResourceName(name)); | ||
| final resource = registration.rpcClientCreator(name.name, _channel); |
There was a problem hiding this comment.
After a reconnect, refresh() replaces _manager with a new ResourceManager built only from server-returned resources. On-demand resources added to the old _manager by this path are not carried over to the new manager, and _resetResourceChannel is never called for them (it only iterates responseNames). Any client reference the caller is holding after a reconnect will have a stale channel and will silently fail until they call getResource again (which re-creates on-demand). This is an inconsistency vs. server-known resources, which do get their channels reset. At minimum the docstring should note this, and the reconnect test in the test plan should be exercised.
| // used by RobotClient.getResource(). Since RobotClient requires a | ||
| // live connection, we test the same code path through its components: | ||
| // ResourceManager + Registry. | ||
|
|
There was a problem hiding this comment.
The test group title says "RobotClient getResource on-demand creation" but none of the tests call RobotClient.getResource() — they reproduce its internal logic inline via ResourceManager + Registry. The actual method under test is never exercised. This means a regression in how RobotClient.getResource assembles those calls (e.g. wrong field used as the name, wrong channel passed) would not be caught. Ideally these tests should be integration-style tests that call robot.getResource<T>(name) on a real or fake RobotClient.
There was a problem hiding this comment.
Code Review — RSDK-10220
lib/src/gen/ was not modified. ✓
Fixes committed (399749a)
- Unused import removed —
import 'package:viam_sdk/src/components/sensor/sensor.dart'was dead code in the test file. - Async assertion fixed —
expect(() => client.position(), throwsA(anything))was replaced withawait expectLater(client.position(), throwsA(anything)). The original form does not properly await the Future returned by the gRPC call; the test could pass vacuously before the async failure was observed.
Design concerns (inline comments)
-
Changed error contract for unregistered subtypes (
client.dart:267) — When the subtype is unknown,Registry.lookupSubtypethrows a rawException, notResourceNotFoundException. Callers that were catchingResourceNotFoundExceptionto handle "resource not found" will silently miss the unregistered-subtype case. Should either re-throw asResourceNotFoundExceptionor be explicitly documented. -
On-demand resources lose their channel after reconnect (
client.dart:269) —refresh()replaces_managerwholesale from server-returned resources; on-demand entries are dropped and_resetResourceChannelis never called for them. After a reconnect, any caller holding a reference to an on-demand client will have a stale channel reference until they callgetResourceagain. Server-known resources are handled consistently via_resetResourceChannel; on-demand ones are not. At minimum this should be documented in the docstring. -
Tests bypass the actual method (
client_test.dart:21) — All five test cases reproduce the internals ofRobotClient.getResource()inline rather than calling the method itself. A copy-paste error in the implementation (e.g. using the wrong field for the resource name) would go undetected. Tests should exerciserobot.getResource<T>(name)directly. -
resource as Tunchecked cast (client.dart:271) — Consistent with howResourceManager.getResourcealready behaves; confirmed intentional.
Summary
RobotClient.getResource()now creates resource clients on-demand using theRegistrywhen they are not found in theResourceManager, instead of throwingResourceNotFoundExceptionDRI
@allisonschiang is the responsible engineer for this PR.
Test plan
make format,make analyze,make testall passJira: RSDK-10220
🤖 Generated with Claude Code