RSDK-10305 resources should support close#387
RSDK-10305 resources should support close#387stuqdog wants to merge 2 commits intoviamrobotics:mainfrom
Conversation
| 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
| } | ||
|
|
||
| // 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.
|
this is getting closed as won't do, with the recommendation that such logic should probably go in a destructor implementation |
3 participants
closemethod to resourcescloseon a resource during a module reconfigure call if the method doesn't havereconfigureimplemented