ServiceConfig duplicate export produces misleading exception

[JerryChin]

I mistakenly exported ServiceConfig twice, the second call produces the following exception which makes no sense:

java.lang.reflect.InvocationTargetException: null
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_91]
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_91]
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_91]
	at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_91]
	at org.apache.dubbo.config.AbstractConfig.refresh(AbstractConfig.java:568) [dubbo-2.7.0.jar:2.7.0]
	at org.apache.dubbo.config.ServiceConfig.checkAndUpdateSubConfigs(ServiceConfig.java:272) [dubbo-2.7.0.jar:2.7.0]
	at org.apache.dubbo.config.ServiceConfig.export(ServiceConfig.java:328) [dubbo-2.7.0.jar:2.7.0]
	at xx.xx.xx.xx.rpc.NodeRpcServerV3.start(NodeRpcServerV3.java:298) [classes/:?]
	at xx.xx.xx.xx.rpc.NodeRpcV3Test.init(NodeRpcV3Test.java:21) [test-classes/:?]
	....
Caused by: java.lang.IllegalArgumentException: Unsupported generic type false
	at org.apache.dubbo.config.ServiceConfig.setGeneric(ServiceConfig.java:959) ~[dubbo-2.7.0.jar:2.7.0]
	... 67 more

It took me at least one hour to locate the root cause.

Dubbo version: 2.7.0

I've crafted a demo for this issue at this repo.

[kexianjun]

sure this exception is misleading,do you have any idea to improve this?

[JerryChin]

@kexianjun

I do have some ideas after reviewing the source code of ServiceConfig.

First, the existing implementation by using two variables for controlling ServiceConfig exporting state is not only awkward to code, but also can theoretically lead invalid state transitions (exported and export both simultaneously true or false).

Secondly, the exported is set in doExport() method which could be called asynchronously, this makes it hard to refactor to prevent duplication exporting.

Okay, there's not "last but not least".

I'll suggest to introduce the following changes:

1. refactor the two variables aforementioned into one AtomicInteger variable.
2. compare and set exporting state in export() and unexport() methods before doing anything.
3. it's obviously to see that we can now safely remove the synchronized from the two methods.

[kexianjun]

@kexianjun

I do have some ideas after reviewing the source code of ServiceConfig.

First, the existing implementation by using two variables for controlling ServiceConfig exporting state is not only awkward to code, but also can theoretically lead invalid state transitions (exported and export both simultaneously true or false).

Secondly, the exported is set in doExport() method which could be called asynchronously, this makes it hard to refactor to prevent duplication exporting.

Okay, there's not "last but not least".

I'll suggest to introduce the following changes:

1. refactor the two variables aforementioned into one AtomicInteger variable.
2. compare and set exporting state in export() and unexport() methods before doing anything.
3. it's obviously to see that we can now safely remove the synchronized from the two methods.

could you pls send a pr about your suggestion?

[JerryChin]

I'll try to work out a PR for this issue this evening. @kexianjun

[tswstarplanet]

I think PR 3629 doesn't catch the reason. I submit a new pr for the problem.