Make UserThread::run* methods thread safe #4122
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The runAfter* methods delegate to UITimer::run(Later|Periodically) in the case of the desktop application. These use the JavaFX TimeLine API (via bisq.common.reactfx.FXTimer) to schedule future events. However, this API isn't thread safe and isn't meant to be called outside the FX application thread. This causes occasional misfirings and out-of-order scheduling when UserThread::runAfter is called outside the user thread. Make the UITimer::run* methods safe to call from any thread by checking we are in the application thread and delegating to UserThread::execute otherwise. This also improves consistency between the contracts of the runAfter* and execute methods. As the former has many call sites, this is safer than trying to track down all the non-thread-safe uses. (The Timer used in the headless app already appears to be thread-safe.) This fixes bisq-network#4055 (Bisq sometimes fails to prompt user for password to unlock wallet), caused by out-of-order scheduling of the execute and runAfter tasks in the WalletConfig.onSetupCompleted anonymous class method in bisq.core.btc.setup.WalletsSetup.initialize. Also prevent an exception caused by non-thread-safe calls into JavaFX during the shutdown of OpenOfferManager, which was uncovered by the above, by adding a missing UserThread::execute call.
stejbac
changed the title
ripcurlx
added
the
is:priority
freimair
approved these changes
Apr 10, 2020
There was a problem hiding this comment.
Also prevent an exception caused by non-thread-safe calls into JavaFX during the shutdown of OpenOfferManager, which was uncovered by the above, by adding a missing UserThread::execute call.
I was going to address that and other things with bisq-network/projects#28. But that is a good fix until then.
to the topic:
- reviewed the code
- did a local build
- did a smoke test checking for any irregularities
all went good. Its a 👍 from me.
ripcurlx
approved these changes
Apr 10, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
UserThread::run*methods delegate toUITimer::run(Later|Periodically)in the case of the desktop application. These use the JavaFXTimeLineAPI (viabisq.common.reactfx.FXTimer) to schedule future events. However, this API isn't thread safe and isn't meant to be called outside the FX application thread. This causes occasional task misfires and out-of-order scheduling whenUserThread::runAfteris called outside the user thread.Make the
UITimer::run*methods safe to call from any thread by checking we are in the application thread and delegating toUserThread::executeotherwise. This also improves consistency between the contracts of therun*andexecutemethods. As the former has many call sites, this is safer than trying to track down all the non-thread-safe uses.(The
Timerused in the headless app already appears to be thread-safe.)This fixes #4055, caused by out-of-order scheduling of the
executeandrunAftertasks in theWalletConfig.onSetupCompletedanonymous class method inbisq.core.btc.setup.WalletsSetup.initialize.Also prevent an exception caused by non-thread-safe calls into JavaFX during the shutdown of
OpenOfferManager, which was uncovered by the above, by adding a missingUserThread::executecall.