search

jurmous / etcd4j

Java / Netty client for etcd, the highly-available key value store for shared configuration and service discovery.
Apache License 2.0
267 stars 83 forks source link

Replace recursion with while-loop when retrying #152

Closed shiroari closed 7 years ago

shiroari commented 7 years ago

Recursive get function in ResponsePromise can cause stack overflow error while retrying. This commit replaces recursion with simple while-loop. See issue #122

lburgazzoli commented 7 years ago

Can you add some test ?

shiroari commented 7 years ago

@lburgazzoli PR doesn't introduce new functionality and doesn't change behavior, so it is supposed to be covered by existing tests. But I am personally missing a related test case as well. I couldn't find any unit test that tests failures or retries and I can not see a good way how it could be done with current setup. Do you mind to add mockito and probably some testing rest libraries like restito to dependencies?

lburgazzoli commented 7 years ago

I've asked for a reproducer in #122 and I had no time to write one so you are welcome to provide it as well as adding the required dependencies.

shiroari commented 7 years ago

Unfortunately there is no way to test 'stack-overflow' case directly. I can cover only that retrying works properly. In order to address 'stack-overflow' problem and similar problems I suggest you to add some java static analysis tool to project.

lburgazzoli commented 7 years ago

Yeah I mean a test that cover the retry case.

shiroari commented 7 years ago

Added a few test cases

lburgazzoli commented 7 years ago

thx for the pr