fix(#10831): When using the deregisterInstance method to remove one o…

[Bo-Qiu]

What is the purpose of the change

fix(#10831 ) When using the deregisterInstance method to remove one of multiple instances registered by batchRegisterInstance, all instances registered by batchRegisterInstance will be removed.

Brief changelog

fix(#10831 ) When using the deregisterInstance method to remove one of multiple instances registered by batchRegisterInstance, all instances registered by batchRegisterInstance will be removed.

Verifying this change

fix(#10831 ) When using the deregisterInstance method to remove one of multiple instances registered by batchRegisterInstance, all instances registered by batchRegisterInstance will be removed.

[codecov-commenter]

Codecov Report

Merging #10836 (c7603bb) into develop (9561db3) will decrease coverage by 0.30%.
Report is 7 commits behind head on develop.
The diff coverage is 48.32%.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10836      +/-   ##
=============================================
- Coverage      53.46%   53.17%   -0.30%     
+ Complexity      5626     5619       -7     
=============================================
  Files            923      927       +4     
  Lines          29458    29555      +97     
  Branches        3244     3249       +5     
=============================================
- Hits           15751    15717      -34     
- Misses         12330    12452     +122     
- Partials        1377     1386       +9     

Files Changed	Coverage Δ
...ibaba/nacos/common/paramcheck/ParamCheckRules.java	0.00% <ø> (ø)
...a/nacos/common/paramcheck/ParamCheckerManager.java	0.00% <0.00%> (ø)
.../nacos/console/filter/ConsoleParamCheckFilter.java	0.00% <0.00%> (ø)
...e/paramcheck/ConsoleDefaultHttpParamExtractor.java	20.00% <0.00%> (-1.43%)	⬇️
...os/core/paramcheck/AbstractHttpParamExtractor.java	40.00% <0.00%> (-4.45%)	⬇️
...cos/core/paramcheck/AbstractRpcParamExtractor.java	100.00% <ø> (ø)
...acos/core/paramcheck/RpcParamExtractorManager.java	76.19% <0.00%> (ø)
...e/remote/grpc/GrpcServerParamCheckInterceptor.java	4.34% <0.00%> (-0.92%)	⬇️
...ng/paramcheck/NamingDefaultHttpParamExtractor.java	6.97% <0.00%> (-0.53%)	⬇️
...ramcheck/NamingInstanceBeatHttpParamExtractor.java	14.28% <0.00%> (-0.53%)	⬇️
... and 18 more

... and 3 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8adca68...c7603bb. Read the comment docs.

[KomachiSion]

Is it thread safe?

[Bo-Qiu]

To ensure the thread safety of this method, we just need to maintain the thread safety of batchDeregisterService. Is the thread safety issue around batchDeregisterService fixed within this PR, or should we propose a new PR for it?

[Bo-Qiu]

@KomachiSion
By the way, I've noticed that there are many places where unnecessary 'synchronized' locks have been added. Since registeredInstances and subscribes are instances of ConcurrentHashMap, which is inherently thread-safe, there's no need to add extra locks for its atomic operations. Do you think it's necessary to open an issue to optimize this?
e.g.

synchronized (subscribes) {
            subscribes.put(key, redoData);
        }

synchronized should be removed.

synchronized (subscribes) {
            SubscriberRedoData redoData = subscribes.get(key);
            if (null != redoData) {
                redoData.setRegistered(true);
            }
        }

It can be replaced by the computeIfPresent method

[Bo-Qiu]

I have directly placed the implementation of batchDeregisterService within a synchronized block to ensure thread safety. Should I use a better way to ensure thread safety?