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

method createServiceIfAbsent in ServiceManager require sync #3922

Closed
robinZhao opened this issue Sep 28, 2020 · 3 comments
Closed

method createServiceIfAbsent in ServiceManager require sync #3922

robinZhao opened this issue Sep 28, 2020 · 3 comments
Labels
contribution welcome follow up this problem requires continuous follow-up kind/enhancement Category issues or prs related to enhancement.
Milestone

Comments

@robinZhao
Copy link

robinZhao commented Sep 28, 2020

method createServiceIfAbsent in ServiceManager have possibility of concurrent creation
require a sync here

public void createServiceIfAbsent(String namespaceId, String serviceName, boolean local, Cluster cluster)
throws NacosException {
Service service = getService(namespaceId, serviceName);
if (service == null) {
synchronized (this){
service = getService(namespaceId, serviceName);
if(null!=service){
return;
}
Loggers.SRV_LOG.info("creating empty service {}:{}", namespaceId, serviceName);
service = new Service();
service.setName(serviceName);
service.setNamespaceId(namespaceId);
service.setGroupName(NamingUtils.getGroupName(serviceName));
// now validate the service. if failed, exception will be thrown
service.setLastModifiedMillis(System.currentTimeMillis());
service.recalculateChecksum();
if (cluster != null) {
cluster.setService(service);
service.getClusterMap().put(cluster.getName(), cluster);
}
service.validate();
putServiceAndInit(service);
if (!local) {
addOrReplaceService(service);
}
}
}
}

Describe the bug
A clear and concise description of what the bug is.

Expected behavior
A clear and concise description of what you expected to happen.

Acutally behavior
A clear and concise description of what you actually to happen.

How to Reproduce
Steps to reproduce the behavior:

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Desktop (please complete the following information):

  • OS: [e.g. Centos]
  • Version [e.g. nacos-server 1.3.1, nacos-client 1.3.1]
  • Module [e.g. naming/config]
  • SDK [e.g. original, spring-cloud-alibaba-nacos, dubbo]

Additional context
Add any other context about the problem here.

@KomachiSion
Copy link
Collaborator

Current logic, if there are many thread create Service, they will enter the putServiceAndInit method and then putService method
In putService method, it contain an putServiceLock and make sure the service map only contain one service.
So I think there is no need to lock in outer method

@KomachiSion KomachiSion added the kind/discussion Category issues related to discussion label Oct 12, 2020
@KomachiSion KomachiSion added kind/enhancement Category issues or prs related to enhancement. contribution welcome and removed kind/discussion Category issues related to discussion labels Jan 4, 2021
@KomachiSion
Copy link
Collaborator

I read the source code again, and found that there are some problem indeed. We will enhance this problem.

@KomachiSion KomachiSion added this to the 1.4.1 milestone Jan 4, 2021
@KomachiSion KomachiSion added the follow up this problem requires continuous follow-up label Jan 4, 2021
@KomachiSion KomachiSion modified the milestones: 1.4.1, 1.4.2 Jan 13, 2021
@a7217107
Copy link
Contributor

i will solve it

KomachiSion pushed a commit that referenced this issue Jan 18, 2021
…sync (#4713)

* fix_#3922

method createServiceIfAbsent in ServiceManager require sync #3922

* roll back createServiceIfAbsent
wjm0729 added a commit to wjm0729/nacos that referenced this issue Jan 21, 2021
* 'develop' of https://github.com/alibaba/nacos:
  Use SafeConstructor to parse yaml configuration for AbstractConfigChangeListener (alibaba#4753)
  [ISSUE alibaba#3922] method createServiceIfAbsent in ServiceManager require sync (alibaba#4713)
  fix cloning configuration last desc (alibaba#4697)
  [ISSUE alibaba#4661]configServletInner#doGetConfig code optimization (alibaba#4666)
  Use revision to set version and upgrade to 1.4.2-SNAPSHOT (alibaba#4711)
  Revert interceptor all ui problem
  Fix alibaba#4701 (alibaba#4703)
  delete comment.
  fix metadata batch operation may delete instance problem.
  Upgrade to 1.4.1 (alibaba#4695)
  fix alibaba#4686 (alibaba#4692)
  Build nacos console front for 1.4.1 (alibaba#4690)
wjm0729 added a commit to wjm0729/nacos that referenced this issue Jan 21, 2021
…op-import-v1

* 'develop' of https://github.com/alibaba/nacos:
  Use SafeConstructor to parse yaml configuration for AbstractConfigChangeListener (alibaba#4753)
  [ISSUE alibaba#3922] method createServiceIfAbsent in ServiceManager require sync (alibaba#4713)
  fix cloning configuration last desc (alibaba#4697)
  [ISSUE alibaba#4661]configServletInner#doGetConfig code optimization (alibaba#4666)
  Use revision to set version and upgrade to 1.4.2-SNAPSHOT (alibaba#4711)
  Revert interceptor all ui problem
  Fix alibaba#4701 (alibaba#4703)
  delete comment.
  fix metadata batch operation may delete instance problem.
  Upgrade to 1.4.1 (alibaba#4695)
  fix alibaba#4686 (alibaba#4692)
  Build nacos console front for 1.4.1 (alibaba#4690)
  For checkstyle
  fix: fix Jraft WriteRequest type problem.
  Add server identity to replace user-agent white list. (alibaba#4683)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution welcome follow up this problem requires continuous follow-up kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Development

No branches or pull requests

3 participants