Close method on Resolver interface

[jjcollinge]

Ask your question here

I'm not sure if there's already been discussions around this topic (I can't find any) - is there any reason we cannot add a Close() method to the resolver interface to allow any components that use stateful services to unregister? This would allow Consul and mDNS to shutdown correctly without leaving orphaned services. The change would be additive (non-breaking) so was wondering if anyone else had looked into this and found a good reason not to add this?

[daixiang0]

Agree to add it.

[dapr-bot]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged (pinned, good first issue, help wanted or triaged/resolved) or other activity occurs. Thank you for your contributions.

[mukundansundar]

Agree to this.

@dapr/maintainers-components-contrib
Labels to add: P1 size:xs good-first-issue area/runtime/nameresolution triaged/resolved

[jjcollinge]

I'm happy to implement this @mukundansundar - could you please assign me so I can track it.

[mukundansundar]

@jjcollinge please use /assign to assign yourself to the issue.

[jjcollinge]

/assign

[dapr-bot]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged (pinned, good first issue, help wanted or triaged/resolved) or other activity occurs. Thank you for your contributions.

[mukundansundar]

not-stale

[dapr-bot]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged (pinned, good first issue, help wanted or triaged/resolved) or other activity occurs. Thank you for your contributions.

[ItalyPaleAle]

@berndverst We don't necessarily need to update the interface for this. It can be implemented as an optional Close() error method.

The runtime is already checking, upon termination, if components implement io.Closer (e.g. Close() error), and if they do, it invokes them:

https://github.com/dapr/dapr/blob/ccce9e451a4b96d1d21a7688196ee7aa89654d6f/pkg/runtime/runtime.go#L2652-L2660

[berndverst]

@berndverst We don't necessarily need to update the interface for this. It can be implemented as an optional Close() error method.

The runtime is already checking, upon termination, if components implement io.Closer (e.g. Close() error), and if they do, it invokes them:

https://github.com/dapr/dapr/blob/ccce9e451a4b96d1d21a7688196ee7aa89654d6f/pkg/runtime/runtime.go#L2652-L2660

That works.

[ItalyPaleAle]

I think this issue can be closed.

mDNS already implements the Close() method that disposes of resources.

Consul doesn't, but if that's needed, IMHO that's an issue specific to the component and not about the general interface for the resolvers. So should be a separate issue...

[artursouza]

It was done a while ago, marked as closed and moved to 1.10 since @ItalyPaleAle suggested that might be the correct timeline for that change.