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

PriceProvider: Add volatile to cachedResult field #43

Merged

Conversation

alvasw
Copy link
Contributor

@alvasw alvasw commented Nov 16, 2023

The get() and put() calls can happen on different threads. The volatile keyword makes sure that all updates to cachedResult are visible to all threads.

Relates to #33.

The get() and put() calls can happen on different threads. The volatile
keyword makes sure that all updates to cachedResult are visible to all
threads.

Relates to bisq-network#33.
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I disagree with the assertion "can happen on different threads" since a provider is run on a single TimerTask thread.

Regardless, this change seems benign.

ACK.

@alvasw
Copy link
Contributor Author

alvasw commented Nov 17, 2023

Every ExchangeRateProvider starts its own Timer running on its own background thread. When many clients call the REST API at the same time, several threads from the server thread pool handle the requests concurrently. Without the volatile keyword these threads could return a stale state because the last updates are not visible to them yet.

Timer / Background Thread Creation:

timer.scheduleAtFixedRate(new TimerTask() {
@Override
public void run() {
try {
refresh();
} catch (Throwable t) {
// we only log scheduled calls to refresh that fail to ensure that
// the application does *not* halt, assuming the failure is temporary
// and on the side of the upstream price provider, eg. BitcoinAverage
log.warn("refresh failed", t);
}
}
}, refreshInterval.toMillis(), refreshInterval.toMillis());

@ghost
Copy link

ghost commented Nov 17, 2023

ACK

Copy link
Contributor

@gabernard gabernard left a comment

Choose a reason for hiding this comment

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

utACK

@gabernard gabernard merged commit 4a20ab7 into bisq-network:main Dec 30, 2023
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