Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZookeeperRegistryDataCacheImpl may be not threadSafe #32

Open
OrezzerO opened this issue Jul 8, 2019 · 3 comments
Open

ZookeeperRegistryDataCacheImpl may be not threadSafe #32

OrezzerO opened this issue Jul 8, 2019 · 3 comments

Comments

@OrezzerO
Copy link

OrezzerO commented Jul 8, 2019

The way we use ConcurrentHashMap may be not correct.
For example:

 List<RpcProvider> currentProviderList = providers.get(rpcService);
        if (currentProviderList == null) {
            //todo 2019-07-05 11:23 线程安全问题
            providers.put(rpcService, providerList);
        } else {
            for (RpcProvider provider : providerList) {
                if (currentProviderList.contains(provider)) {
                    continue;
                }
                currentProviderList.add(provider);
            }
        }

Sometimes put() method may be invoked twice, it is not we expected.

@chpengzh
Copy link
Member

chpengzh commented Jul 8, 2019

All method are invoked by PathChildrenCacheListener.childEvent, which will be invoke by a single thread with event order. You can debug it or print a thread id to find that.

RegistryDataCache is not thread safe, yes, but it is used in a thread safe case.

@OrezzerO
Copy link
Author

OrezzerO commented Jul 9, 2019

Got it. ZooKeeper watches are single threaded. https://cwiki.apache.org/confluence/display/CURATOR/TN1
So ,it is unnecessary to use ConcurrentHashMap?

@chpengzh
Copy link
Member

chpengzh commented Jul 9, 2019

Although it is written by one and read by others, it is better to be thread safe in my opinion. A better practice is using Map.compute or Map.computeIfAbsent rather than multi changes above.

See sofa-dashboard-client for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants