RSDK-11067: remove resource on alwaysrebuild #458
Conversation
lia-viam
requested review from
gabegottlob and
njooma
and removed request for
a team
src/viam/sdk/module/service.cpp
Outdated
| stoppable->stop(); | ||
| } | ||
|
|
||
| manager->remove(cfg.resource_name()); |
There was a problem hiding this comment.
Below, this calls manager->replace_one. The documentation for replace_one indicates that it will only work right if resource is present, and will fail otherwise. So, by removing the resource here, I think it prevents replace_one below from working.
There was a problem hiding this comment.
ugh good catch...i will look into what's happening in replace_one because i think from @hexbabe earlier testing the destructor on the original instance was not getting called during this always-rebuild control flow
lia-viam
changed the title
|
|
||
| const std::shared_ptr<const ModelRegistration> reg = | ||
| Registry::get().lookup_model(cfg.name()); | ||
| Registry::get().lookup_model(cfg.api(), cfg.model()); |
There was a problem hiding this comment.
If you fail to look up the model registration here, I think you should return an error. It's a pretty strange state of affairs because the model was registered once, apparently, such that the object could be brought into existence (otherwise, what is being reconfigured?) but now that it is time to reconfigure by rebuilding, the registration is gone. That also would have made the error here obvious, rather than a silent misbehavior.
There was a problem hiding this comment.
I think you're right, it was really confusing to debug
src/viam/sdk/module/service.cpp
Outdated
| std::shared_ptr<Resource> resource = reg->construct_resource(deps, cfg); | ||
| manager->replace_one(cfg.resource_name(), std::move(resource)); |
There was a problem hiding this comment.
- Arguably, this is part of RSDK-11067, but the behavior here means that if
construct_resourcefails, the old one sticks around. Is that OK? - Maybe add some warn logging here after the call to
replace_onethat ifres.use_count != 1that the oldResourcehas been held alive somewhere? I know it isn't perfect due to multithreading, but at least it gives some indication that things haven't gone according to plan.
There was a problem hiding this comment.
Wait, I just saw the commit that removed the TODO RSDK-11067, but this review doesn't fix that fully. The call to construct_resource here happens before the call to replace_one which will (in theory) remove the old one. If the intention is to actually resolve RSDK-11067, the ordering here needs to be changed, and I don't think replace_one can be used.
There was a problem hiding this comment.
I think this does resolve RSDK-11067 at least in terms of the observed behavior. Maybe the issue title was described incorrectly, because it would be more accurate to say that in the absence of Reconfigurable the reconfiguration was not happening whatsoever, the old one was not getting deleted, etc.
If we do it without replace_one would you suggest a manual call to remove followed by add ? Because it occurred to me that in the absence of Reconfigurable that a call to ReconfigureResource should be equivalent to RemoveResource followed by AddResource, so maybe we can reuse the code from each of those
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
LGTM, but please file a new ticket about investigating reconfiguration behavior and CC me on it. I'll write up my observations there.
No description provided.