Skip refresh if the previous refresh is ongoing

[alvasw]

Related to #33

[alvasw]

@jmacxx Could you test this PR?

[ghost]

I also thought re-entrancy could have been the problem @alvasw. To investigate (prior to this PR) I made an http server on a separate machine posing as a dummy poloniex provider that would accept the request and block for a while before sending the response. I increased the frequency of refresh() period to 10 seconds while the http call was taking 20 seconds to respond. This was an attempt to reproduce the bug by having multiple refresh() calls ongoing. However this did not happen, in the TimerTask loop a second refresh would not occur until the in-progress one completed. This indicates that TimerTask is a single thread, and documentation agrees. I ran the test for an extended period of time (>24h).

https://stackoverflow.com/questions/2264476/what-happens-if-a-timertask-takes-longer-to-execute-than-the-specified-interval

https://stackoverflow.com/questions/1237804/is-javas-timer-task-guaranteed-not-to-run-concurrently

https://stackoverflow.com/questions/20041713/does-scheduleatfixedrate-call-a-different-thread-of-execution-when-the-current-e

So I think the re-entrancy guard unlikely to fix the issue.

But in absence of other ideas perhaps it is worth deploying it to production anyway. If we do that, there should be a prominent comment next to the re-entrancy guard explaining it, or giving context. Also I think the Refresh in progress. Skipping this iteration message should be warn level, not debug.

[alvasw]

The Timer documentation says that it will try to catch up on missed executions. Let's say a task takes usually 1 second to complete and for some reason it takes 30 seconds once. If we schedule the Timer to run the ask every 10 seconds, the task will be called three times to catch up.

JDK Timer.scheduleAtFixedRate documentation:
"In fixed-rate execution, each execution is scheduled relative to the scheduled execution time of the initial execution. If an execution is delayed for any reason (such as garbage collection or other background activity), two or more executions will occur in rapid succession to "catch up." In the long run, the frequency of execution will be exactly the reciprocal of the specified period (assuming the system clock underlying Object.wait(long) is accurate). [...]"

Source: https://docs.oracle.com/javase/8/docs/api/java/util/Timer.html#scheduleAtFixedRate-java.util.TimerTask-long-long-

[alvasw]

However I don't think that this PR will fix the bug in #33 .

[ghost]

Looking at it from another angle, pricenode provider has an entry point from SpringBoot start(). What if SpringBoot invokes this more than once, from different threads? That could cause a problem as there's currently no guard against that.

void start()
Start this component.
Should not throw an exception if the component is already running.

If you would adapt this PR to guard against re-entrancy from SpringBoot, I think it would be better.

[gabernard]

@alvasw Are you still working on this PR?

[gabernard]

I'm closing this PR for now.