-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[http] fix initial refresh #9626
Conversation
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
@@ -59,6 +59,10 @@ public RefreshingUrlCache(ScheduledExecutorService executor, RateLimitedHttpClie | |||
this.headers = thingConfig.headers; | |||
fallbackEncoding = thingConfig.encoding; | |||
|
|||
// do initial refresh | |||
refresh(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please elaborate why this is necessary?
According to https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/ScheduledThreadPoolExecutor.html
The documentation for scheduleWithFixedDelay
, which is in this case:
executor.scheduleWithFixedDelay(this::refresh, 0, thingConfig.refresh, TimeUnit.SECONDS);
is:
Submits a periodic action that becomes enabled first after the given initial delay, and subsequently with the given delay between the termination of one execution and the commencement of the next.
And initial delay
is already set to 0, so I am wondering why this extra call is necessary? Is this a bug in the JDK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be. All I can say is that the first refresh occurs after the first refresh time is done. Zulu-11 on Windows in my case and openhabian (IIRC) in the reported case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is entirely possible that the executor (which I'm assuming uses a shared threadpool) was already busy at the time so the execution of the initial refresh was delayed until threads were available to execute it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this also happens on my test instance with only the http binding installed, no rules and only one channel. I doubt all threads are busy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right. Even the start with 0. Because it's in the constructor. So when refresh in the constructor (or possible when start time is 0) is called consumers
is still empty (certainly true for refresh call in constructor). So refresh will return directly. Maybe it makes more sense to start with 1 second delay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hilbrand Good analysis! I knew there was something fishy, thanks for nailing it down ;)
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now.
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
As reported in the community forum the first refresh occured after the refreshTime, not immediately after the channel was configured. Especially for channels with a long refreshTime this was very inconvenient.
Signed-off-by: Jan N. Klug jan.n.klug@rub.de