Naming local cache may be ignored in a rare scenario

[nkorange]

Describe the bug

Naming local cache may be ignored in a rare scenario. So even when local cache is not empty, user's invocation would still get exception.

Expected behavior

If local cache is not empty, Nacos client should never throw exception.

Actually behavior

In a rare case, when local cache is not empty, user's invocation would get exception.

How to Reproduce

It's hard to reproduce but it did happen in our production environment. Here is the related logic:

in the method at getServiceInfoBySubscribe:

1. first it tried to get service info from local cache;
2. then it will check if local cache is null or client is not subscribed:

3. if yes, it will try to subscribe from remote Nacos server.

Usually it will work, because whenever clientProxy.isSubscribed(...) returns false, it means the Nacos client has just reconnected to Nacos server and closed the old connection:

As new connection is ready, so the subscribe request would succeed.

But if the new connection is down immediately again so the subscribe request failed, then in the method getServiceInfoBySubscribe, an exception will be thrown.

Desktop (please complete the following information):

• OS: [e.g. Centos]
• Version: nacos-server 2.1.2, nacos-client 2.1.2
• Module: naming
• SDK: spring-cloud-alibaba-nacos 2021.1

Additional context

I suggest to add a protection logic in the method getServiceInfoBySubscribe, so that whenever the local cache is not empty, the remote request error or any other exception will be ignored.

[KomachiSion]

When client call subscribe api to do sub, it should throw exception when connection not ready and subscribe failed, which notify users there is some exception for connection and should retry or do other operation.

If call getAllInstances with subscribe=true, It can be discuss whether it should throw exception when cache exist.

In my option, when subscribe=true, connection not ready should throw exception to notify, because users think getAllInstances is get instances from server, not only cache.

[nkorange]

In current implementation, subscribe=true and Nacos server disconnected would not throw exception. Only when subscribe=true and reconnect succeed and re-subscribe failed would the client throw exception.

[KomachiSion]

In current implementation, subscribe=true and Nacos server disconnected would not throw exception. Only when subscribe=true and reconnect succeed and re-subscribe failed would the client throw exception.

Why subscribe=true and Nacos server disconnected would not throw exception？ I think it also throw exception.

[nkorange]

I have tested it.

1. Start local Nacos server.
2. Register service test.1 to local Nacos server.
3. Run a consumer with the following code:

public static void main(String[] args) throws Exception {
        NamingService namingService = NamingFactory.createNamingService("127.0.0.1:8848");
        while (true) {
            System.out.println(namingService.getAllInstances("test.1", true));
            Thread.sleep(5000L);
        }
    }

4. shutdown Nacos server.

The consumer code can still get the instance list of test.1

[nkorange]

I suggest to change the method getServiceInfoBySusbcribe to the following logic:

private ServiceInfo getServiceInfoBySubscribe(String serviceName, String groupName, String clusterString,
            boolean subscribe) throws NacosException {
        ServiceInfo serviceInfo;
        if (subscribe || !EnvUtil.isDirectQueryEnabled()) {
            serviceInfo = serviceInfoHolder.getServiceInfo(serviceName, groupName, clusterString);
            if (null == serviceInfo || !clientProxy.isSubscribed(serviceName, groupName, clusterString)) {
                try {
                    serviceInfo = clientProxy.subscribe(serviceName, groupName, clusterString);
                } catch (NacosException ne) {
                    NAMING_LOGGER.warn("getServiceInfoBySubscribe subscribe failed, {}", serviceName);
                    // 只有订阅失败，且本地缓存也为空的情况下，才抛异常：
                    if (serviceInfo == null) {
                        throw ne;
                    }
                }
            }
        } else {
            serviceInfo = clientProxy.queryInstancesOfService(serviceName, groupName, clusterString, 0, false);
        }
        return serviceInfo;
    }

[KomachiSion]

I see, it might be when first time call clientProxy.subscribe, clientProxy.isSubscribed will return true so that loop call will not entry condition.

And when first call while starting, it must be return false and entry condition to call clientProxy.subscribe and throw exception by connection loss.

I think your changes is ok. just do some enhance for Method complexity, such as extract to a new method to cover the logic.

[BDL-LTD]

Describe the bug

Naming local cache may be ignored in a rare scenario. So even when local cache is not empty, user's invocation would still get exception.

Expected behavior

If local cache is not empty, Nacos client should never throw exception.

Actually behavior

In a rare case, when local cache is not empty, user's invocation would get exception.

How to Reproduce

It's hard to reproduce but it did happen in our production environment. Here is the related logic:

in the method at [email protected]:

1. first it tried to get service info from local cache;
2. then it will check if local cache is null or client is not subscribed:

3. if yes, it will try to subscribe from remote Nacos server.
Usually it will work, because whenever clientProxy.isSubscribed(...) returns false, it means the Nacos client has just reconnected to Nacos server and closed the old connection:

As new connection is ready, so the subscribe request would succeed.

But if the new connection is down immediately again so the subscribe request failed, then in the method getServiceInfoBySubscribe, an exception will be thrown.

Desktop (please complete the following information):

• OS: [MR BLAKE DAMIAN LING]
• Version: nacos-server 2.1.2, nacos-client 2.1.2
• Module: naming
• SDK: spring-cloud-alibaba-nacos 2021.1

Additional context

I suggest to add a protection logic in the method getServiceInfoBySubscribe, so that whenever the local cache is not empty, the remote request error or any other exception will be ignored.