Should org.apache.dubbo.registry.support.AbstractRegistry#getRegistered returns unmodifiable sets?

[kezhenxu94]

I notice that the method org.apache.dubbo.registry.support.AbstractRegistry#getRegistered just returns the internal reference of registered, and everyone can modify this Set outside the Registry, is this a good practice?

An example is that in org.apache.dubbo.registry.support.AbstractRegistryTest#testRegister, there is something like:

//test multiple urls
abstractRegistry.getRegistered().clear();

we clear the registered urls outside the Registry, maybe it's not a big deal because we just do some logs and remove the url in unregister method:

    @Override
    public void unregister(URL url) {
        if (url == null) {
            throw new IllegalArgumentException("unregister url == null");
        }
        if (logger.isInfoEnabled()) {
            logger.info("Unregister: " + url);
        }
        registered.remove(url);
    }

but still, it'll lose some logs and if we add more logics in the unregister method in the future, it could be dangerous.

I suggest returning a unmodifiable set in the getRegistered method and unregister all urls one by one or providing a method such as unregisterAll.

There is the same problem with method getSubscribed, getNotified, etc.

What do you think?

[khanimteyaz]

@kezhenxu94 , what you mentioned I am with it. I think we should keep the responsibility of operation on objects e.g. in this case clearing the registry to the owner of registries object AbstractRegistry.java and when ever it returns any mutable member variable it should return copy of it. But having said that we also needs to consider memory footprint/performance of creating clone copy of returned object. I beleve it should not change memory footprint or performance drastically but better to have some data around it.

[kezhenxu94]

I beleve it should not change memory footprint or performance drastically but better to have some data around it.

@khanimteyaz That's my opinion too. What I suggest is to wrap the member variable such as using Collections.unmodifiableSet, and this may break something, so it needs investigation and discussion

[khanimteyaz]

I beleve it should not change memory footprint or performance drastically but better to have some data around it.

@khanimteyaz That's my opinion too. What I suggest is to wrap the member variable such as using Collections.unmodifiableSet, and this may break something, so it needs investigation and discussion

@kezhenxu94 agree.

[beiwei30]

then, can someone nicely work on it?

[kezhenxu94]

@beiwei30 I'll work on it