RSDK-10305 resources should support close #387
Conversation
| @@ -115,6 +115,7 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { | |||
| Registry::get().lookup_model(cfg.name()); | |||
| if (reg) { | |||
| try { | |||
| res->close(); | |||
There was a problem hiding this comment.
@randhid per what you were discussing in slack, if we're trying to reconfigure a modular resource that doesn't implement reconfigure we will now 1) stop it if it's stoppable, 2) call its close method always, and then 3) rebuild it
| @@ -26,5 +26,8 @@ Name Resource::get_resource_name() const { | |||
| return get_resource_name(kResource); | |||
| } | |||
|
|
|||
| // default behavior is to just return | |||
| void Resource::close() {} | |||
There was a problem hiding this comment.
@randhid this was the implementation detail I wanted to run by you. looking at golang, the default behavior for close is a no-op. is that desired here as well?
There was a problem hiding this comment.
Is it true that that is the default behaviour? Mind pointing me to where you are seeing that?
My experience is needing to opt-into the no-op behaviour by embedding a resource.TriviallyCloseable in my resource class, or opting into always rebuild behaviour by including resource.AlwaysRebuild, my understanding hwne having to implement go modules is to have to implement close using those two or a Close implementation, or it just won't build.
There was a problem hiding this comment.
Ahh no, I'm mistaken. The default is a no-op for the TriviallyCloseable specifically, you are correct!
by embedding a
resource.TriviallyCloseablein my resource class, or opting into always rebuild behaviour by includingresource.AlwaysRebuild
I think I'm a little confused by this, how does AlwaysRebuild affect the Close behavior? since a Resource needs to define Close and Close could certainly be called outside the context of a Reconfigure, doesn't including AlwaysRebuild still require us to define Close?
There was a problem hiding this comment.
I'm sorry I should have been clearer. I was trying to explain that there are two embeddings available from the resource package: TriviallyCloseable, which provides a no-op closer, and AlwaysRebuild, which forces a close and rebuild flow. You are correct - AlwaysRebuild requires you to implement a Close.
closemethod to resourcescloseon a resource during a module reconfigure call if the method doesn't havereconfigureimplemented