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

ReferenceConfig initialized not changed to false once subscribe throws exception #4067

Merged
merged 1 commit into from
May 17, 2019
Merged

ReferenceConfig initialized not changed to false once subscribe throws exception #4067

merged 1 commit into from
May 17, 2019

Conversation

haiyang1985
Copy link
Member

@haiyang1985 haiyang1985 commented May 15, 2019

What is the purpose of the change

fix issue #4068

ReferenceConfigCache is able to initialize ReferenceConfig for multiple times. It cannot set initialize to false once subscribe throws exception.

ReferenceConfigCache cache = ReferenceConfigCache.getCache();
DemoService demoService = (DemoService) cache.get(reference);
System.out.println(demoService.sayHello("hi"));

Brief changelog

Set initialized to true after the init finished. Currently, initialized has not been changed to false, if refprotocol.refer throws exception.

if (urls.size() == 1) {
    invoker = refprotocol.refer(interfaceClass, urls.get(0));
}

Verifying this change

  1. Start consumer without provider, it should throw exception from cache.get(reference).
  2. Start consumer with provider, it shows result from the invocation.

Note:
Found another existing issue, that the urls would cause OOM once retries again and again without service provider. I will consider and send another PR for this.

urls.add(u.addParameterAndEncoded(Constants.REFER_KEY, StringUtils.toQueryString(map)));

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a GITHUB_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GITHUB issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [Dubbo-XXX] Fix UnknownException when host config not exist #XXX. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Run mvn clean install -DskipTests=false & mvn clean test-compile failsafe:integration-test to make sure unit-test and integration-test pass.
  • If this contribution is large, please follow the Software Donation Guide.

@ralf0131
Copy link
Contributor

Hi, I could not reproduce the issue you are describing. I want to make sure I understand correctly.

My demo Consumer code:

    public static void main(String[] args) {

        ReferenceConfig<DemoService> referenceConfig = new ReferenceConfig<>();
        referenceConfig.setApplication(new ApplicationConfig("demo-consumer"));
        referenceConfig.setRegistry(new RegistryConfig("nacos://127.0.0.1:8848"));
        referenceConfig.setInterface(DemoService.class);
        ReferenceConfigCache cache = ReferenceConfigCache.getCache();
        while(true) {
            try {
                Thread.sleep(3000);
                DemoService demoService = cache.get(referenceConfig);
                String hello = demoService.sayHello("world");
                System.out.println("result: " + hello);

            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    }

Steps I tried:

  1. Start consumer, the follow exception will be thrown:
java.lang.IllegalStateException: Failed to check the status of the service org.apache.dubbo.demo.DemoService. No provider available for the service org.apache.dubbo.demo.DemoService from the url registry://127.0.0.1:8848/org.apache.dubbo.registry.RegistryService?application=demo-consumer&cluster=registryaware&dubbo=2.0.2&interface=org.apache.dubbo.demo.DemoService&lazy=false&methods=sayHello&pid=68747&qos.port=33333&refer=application%3Ddemo-consumer%26dubbo%3D2.0.2%26interface%3Dorg.apache.dubbo.demo.DemoService%26lazy%3Dfalse%26methods%3DsayHello%26pid%3D68747%26qos.port%3D33333%26register.ip%3D10.0.0.10%26side%3Dconsumer%26sticky%3Dfalse%26timestamp%3D1558016001986&register.ip=10.0.0.10&registry=nacos&side=consumer&sticky=false&timestamp=1558016001986 to the consumer 10.0.0.10 use dubbo version 
	at org.apache.dubbo.config.ReferenceConfig.createProxy(ReferenceConfig.java:419)
	at org.apache.dubbo.config.ReferenceConfig.init(ReferenceConfig.java:330)
	at org.apache.dubbo.config.ReferenceConfig.get(ReferenceConfig.java:250)
	at org.apache.dubbo.config.utils.ReferenceConfigCache.get(ReferenceConfigCache.java:107)
	at org.apache.dubbo.demo.consumer.Application.main(Application.java:46)
  1. Start the provider, the consumer application will soon receive response from the provider:
result: Hello world, response from provider: xx.xx.xx.xx:20890

The behavior looks correct, so what is your actually issue?

@haiyang1985
Copy link
Member Author

@ralf0131 , what's the registry for you? zookeeper cannot reproduce the issue, as it won't throw exception even no provider exists. You can reproduce it once you have thrown exception from the service subscribe.

@ralf0131
Copy link
Contributor

ralf0131 commented May 17, 2019

@ralf0131 , what's the registry for you? zookeeper cannot reproduce the issue, as it won't throw exception even no provider exists. You can reproduce it once you have thrown exception from the service subscribe.

I used Nacos as registry, please refer to my consumer code. What registry did you use?

Did you have other configuration such as xml? Please attach them as well.

@haiyang1985
Copy link
Member Author

haiyang1985 commented May 17, 2019

It's our own registry. Your consumer code is ok. Will Nacos throw exception if no providers? You can reproduce it, when registry throws out exception with no providers as below.

    // Throw out the exception to trigger dubbo local cache instance in startup.
    if (event == null && CollectionValues.isNullOrEmpty(validInstances)) {
      throw new IllegalStateException(getNoInstanceInStartup(consumerURL));
    }

@haiyang1985
Copy link
Member Author

Throw out exception from registry is just part of my points. We cannot predict any exception from the consumer initialization. Currently, initialized flag have been set to false only if invoker initialized. But the invoker maybe not able to be initialized, as any exception could be thrown from below extension.

if (urls.size() == 1) {
   invoker = refprotocol.refer(interfaceClass, urls.get(0));
}

@ralf0131
Copy link
Contributor

It's our own registry. Will Nacos throw exception if no providers?

Yes, it will throw exception, the stack trace is attached in my previous comment. However, when the provider is on, the consumer can receive response from provider. This behavior is different from what you are describing.

You can reproduce it with exception as below from service subscribe.

    // Throw out the exception to trigger dubbo local cache instance in startup.
    if (event == null && CollectionValues.isNullOrEmpty(validInstances)) {
      throw new IllegalStateException(getNoInstanceInStartup(consumerURL));
    }

Where is these lines of code? Is it your customized code that not belong to Dubbo?

@haiyang1985
Copy link
Member Author

It's our own registry. Will Nacos throw exception if no providers?

Yes, it will throw exception, the stack trace is attached in my previous comment. However, when the provider is on, the consumer can receive response from provider. This behavior is different from what you are describing.

You can reproduce it with exception as below from service subscribe.

    // Throw out the exception to trigger dubbo local cache instance in startup.
    if (event == null && CollectionValues.isNullOrEmpty(validInstances)) {
      throw new IllegalStateException(getNoInstanceInStartup(consumerURL));
    }

Where is these lines of code? Is it your customized code that not belong to Dubbo?

Yes, we have extend it with our own registry.

@ralf0131
Copy link
Contributor

if (urls.size() == 1) {
   invoker = refprotocol.refer(interfaceClass, urls.get(0));
}

Again, where is the line of code? Could you specify the location? I suggest you give a complete example instead, rather that the code snipped.

@ralf0131
Copy link
Contributor

It's our own registry. Will Nacos throw exception if no providers?

Yes, it will throw exception, the stack trace is attached in my previous comment. However, when the provider is on, the consumer can receive response from provider. This behavior is different from what you are describing.

You can reproduce it with exception as below from service subscribe.

    // Throw out the exception to trigger dubbo local cache instance in startup.
    if (event == null && CollectionValues.isNullOrEmpty(validInstances)) {
      throw new IllegalStateException(getNoInstanceInStartup(consumerURL));
    }

Where is these lines of code? Is it your customized code that not belong to Dubbo?

Yes, we have extend it with our own registry.

Could you try with some open source registry like ZK or Nacos? Why they are working? If the error is only happen with your own registry. I think it is something that you should improve your own registry.

@haiyang1985
Copy link
Member Author

haiyang1985 commented May 17, 2019

Again, where is the line of code? Could you specify the location? I suggest you give a complete example instead, rather that the code snipped.

It's dubbo's code. When refprotocol.refer throws exception, initialized cannot be set to false. My example cannot work in your side, as it relies our company's registry.

public class ReferenceConfig<T> extends AbstractReferenceConfig {

    private T createProxy(Map<String, String> map) {
        if (isJvmRefer) {
            ...
        } else {
            if (urls.size() == 1) {
                invoker = refprotocol.refer(interfaceClass, urls.get(0));
            } else {
               ...
            }
        }

        if (c && !invoker.isAvailable()) {
            // make it possible for consumer to retry later if provider is temporarily unavailable
            initialized = false;
            ...
        }

        // create service proxy
        return (T) proxyFactory.getProxy(invoker);
    }
}

@haiyang1985
Copy link
Member Author

Could you try with some open source registry like ZK or Nacos? Why they are working? If the error is only happen with your own registry. I think it is something that you should improve your own registry.

I have tried ZK, cannot reproduce as ZK won't throw exception without providers. Let's talk about it in DingDign, my account is haiyang198542.

@ralf0131
Copy link
Contributor

Could you try with some open source registry like ZK or Nacos? Why they are working? If the error is only happen with your own registry. I think it is something that you should improve your own registry.

I have tried ZK, cannot reproduce as ZK won't throw exception without providers. Let's talk about it in DingDign, my account is haiyang198542.

I think I have got what you mean, I am trying to reproduce in my local env.

@ralf0131
Copy link
Contributor

Hi, I did some tweak in my local env and successfully reproduced the issue you describe.

What I did in org.apache.dubbo.registry.integration.RegistryProtocol#doRefer, I add some testing code:

        if (!directory.isAvailable()) {
            throw new RpcException("test");
        }

I confirmed your proposed change works.

Copy link
Contributor

@ralf0131 ralf0131 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the pull request!

@ralf0131 ralf0131 merged commit 8646277 into apache:master May 17, 2019
@haiyang1985 haiyang1985 deleted the dubbo-configCache branch September 12, 2019 08:10
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

Successfully merging this pull request may close these issues.

2 participants