If the client already disconnect, ignore this request to avoid NPE. #6175

horizonzy · 2021-06-27T03:16:45Z

What is the purpose of the change

The client may be null after disconnect, fix it.

KomachiSion · 2021-06-28T02:22:49Z

.../java/com/alibaba/nacos/naming/core/v2/service/impl/EphemeralClientOperationServiceImpl.java

@@ -49,6 +49,9 @@ public EphemeralClientOperationServiceImpl(ClientManagerDelegate clientManager)
    public void registerInstance(Service service, Instance instance, String clientId) {
        Service singleton = ServiceManager.getInstance().getSingleton(service);
        Client client = clientManager.getClient(clientId);
+        if (null == client || !client.isEphemeral()) {


abstract a method named validateClient?
And print an warn log, I think better.

print an warn log, ok.
abstract method validateClient I think is not necessary, If do it, it will be take over by util class, because need check client is null firstly. So I think it not necessary.

Why take over by util class? just add one private method in EphemeralClientOperationServiceImpl is ok. it will means that check the client is valid for EphemeralClientOperationServiceImpl.

Because this judgement not at EphemeralClientOperationServiceImpl, other place also be at same case.

nacos/naming/src/main/java/com/alibaba/nacos/naming/consistency/ephemeral/distro/v2/DistroClientDataProcessor.java

Lines 222 to 235 in e0efe71

public DistroData getDatumSnapshot() {

List<ClientSyncData> datum = new LinkedList<>();

for (String each : clientManager.allClientId()) {

Client client = clientManager.getClient(each);

if (null == client || !client.isEphemeral()) {

continue;

}

datum.add(client.generateSyncData());

}

ClientSyncDatumSnapshot snapshot = new ClientSyncDatumSnapshot();

snapshot.setClientSyncDataList(datum);

byte[] data = ApplicationUtils.getBean(Serializer.class).serialize(snapshot);

return new DistroData(new DistroKey(DataOperation.SNAPSHOT.name(), TYPE), data);

}

before you add in EphemeralClientOperationServiceImpl, only little place need this logic.
After you add, EphemeralClientOperationServiceImpl has at least four place use this logic and both need to print logs. So I think it's time to abstract a method in EphemeralClientOperationServiceImpl. If there are many place in other class need this method. We should abstract to a util class.

KomachiSion · 2021-06-29T01:36:39Z

.../java/com/alibaba/nacos/naming/core/v2/service/impl/EphemeralClientOperationServiceImpl.java

@@ -49,6 +49,10 @@ public EphemeralClientOperationServiceImpl(ClientManagerDelegate clientManager)
    public void registerInstance(Service service, Instance instance, String clientId) {
        Service singleton = ServiceManager.getInstance().getSingleton(service);
        Client client = clientManager.getClient(clientId);
+        if (null == client || !client.isEphemeral()) {
+            Loggers.SRV_LOG.warn("Client connection {} already disconnect", clientId);


The log maybe dismiss maintainer.
It can't include !client.isEphemeral()

Moskalnazar

wow

common/src/main/java/com/alibaba/nacos/common/remote/client/RpcClient.java

…libaba#6175) * if the client already disconnect, ignore this request to avoid npe. * unify code with DistroClientDataProcessor. * print warn info when client already disconnect. * transfer client check logic to method. * if client is not ephemeral, is illegal.

horizonzy added 2 commits June 27, 2021 11:09

if the client already disconnect, ignore this request to avoid npe.

37f72e6

unify code with DistroClientDataProcessor.

db2a6d0

KomachiSion approved these changes Jun 28, 2021

View reviewed changes

KomachiSion requested changes Jun 28, 2021

View reviewed changes

print warn info when client already disconnect.

87249d7

KomachiSion requested changes Jun 29, 2021

View reviewed changes

Moskalnazar reviewed Jun 29, 2021

View reviewed changes

common/src/main/java/com/alibaba/nacos/common/remote/client/RpcClient.java Show resolved Hide resolved

horizonzy added 2 commits June 29, 2021 10:50

transfer client check logic to method.

e2b6567

if client is not ephemeral, is illegal.

7496b6d

KomachiSion approved these changes Jun 30, 2021

View reviewed changes

KomachiSion added kind/refactor kind/enhancement Category issues or prs related to enhancement. labels Jun 30, 2021

KomachiSion added this to the 2.0.3 milestone Jun 30, 2021

KomachiSion merged commit 3fd77bc into alibaba:develop Jun 30, 2021

onewe mentioned this pull request Mar 31, 2022

Nacos Error java.lang.NullPointerException: null #8042

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

If the client already disconnect, ignore this request to avoid NPE. #6175

If the client already disconnect, ignore this request to avoid NPE. #6175

horizonzy commented Jun 27, 2021

KomachiSion Jun 28, 2021 •

edited

Loading

horizonzy Jun 28, 2021

KomachiSion Jun 28, 2021

horizonzy Jun 28, 2021

KomachiSion Jun 29, 2021

KomachiSion Jun 29, 2021

horizonzy Jun 29, 2021

Moskalnazar left a comment

	public DistroData getDatumSnapshot() {
	List<ClientSyncData> datum = new LinkedList<>();
	for (String each : clientManager.allClientId()) {
	Client client = clientManager.getClient(each);
	if (null == client \|\| !client.isEphemeral()) {
	continue;
	}
	datum.add(client.generateSyncData());
	}
	ClientSyncDatumSnapshot snapshot = new ClientSyncDatumSnapshot();
	snapshot.setClientSyncDataList(datum);
	byte[] data = ApplicationUtils.getBean(Serializer.class).serialize(snapshot);
	return new DistroData(new DistroKey(DataOperation.SNAPSHOT.name(), TYPE), data);
	}

If the client already disconnect, ignore this request to avoid NPE. #6175

If the client already disconnect, ignore this request to avoid NPE. #6175

Conversation

horizonzy commented Jun 27, 2021

What is the purpose of the change

KomachiSion Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

horizonzy Jun 28, 2021

Choose a reason for hiding this comment

KomachiSion Jun 28, 2021

Choose a reason for hiding this comment

horizonzy Jun 28, 2021

Choose a reason for hiding this comment

KomachiSion Jun 29, 2021

Choose a reason for hiding this comment

KomachiSion Jun 29, 2021

Choose a reason for hiding this comment

horizonzy Jun 29, 2021

Choose a reason for hiding this comment

Moskalnazar left a comment

Choose a reason for hiding this comment

KomachiSion Jun 28, 2021 •

edited

Loading