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

Make UserThread::run* methods thread safe #4122

Merged

Conversation

stejbac
Copy link
Contributor

@stejbac stejbac commented Apr 6, 2020

The UserThread::run* 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 task misfires 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 run* 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 #4055, 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.

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 stejbac changed the title Make UserThread::runAfter* methods thread safe Make UserThread::run* methods thread safe Apr 6, 2020
@ripcurlx ripcurlx added this to the v1.3.0 milestone Apr 7, 2020
@ripcurlx ripcurlx requested a review from freimair April 7, 2020 14:17
@ripcurlx ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Apr 10, 2020
Copy link
Contributor

@freimair freimair left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK - based on #4122 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:priority PR or issue marked with this label is up for compensation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bisq sometimes fails to prompt user for password to unlock wallet
3 participants