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

Fix initialization ordering issue #5219

Merged
merged 2 commits into from Feb 24, 2021
Merged

Fix initialization ordering issue #5219

merged 2 commits into from Feb 24, 2021

Conversation

ghost
Copy link

@ghost ghost commented Feb 23, 2021

Fixes #5210

TradeManager must be inited before MailboxMessageService. Fixes two NPE scenarios described in #5210

Scenario 1. Start up with a Mailbox message queued for delivery to DisputeManager

  • This relates to the first call stack posted in Buyer opens mediation while seller is offline - NullPointerException #5210.
  • The mailbox message service is initialized and propagates the message synchronously up to DisputeManager
  • DisputeManager queries the Trade associated with the Dispute.
  • We get an NPE while attempting to access Trade -> processModel -> provider as the TradeManager has not yet been initialized via initPersistedTrades.
  • The solution is to bootstrap the MailboxMessageService last so that any messages it dispatches are received by initialized services.

Scenario 2. Start up with a password protected wallet.

  • Same pre-conditions as before, + have password protection set on Bob's wallet.
  • After entering the password we get the second NPE described.
  • The call stack shows that the initialization route located at DomainInitialisation::initDomainServices() is used. It needs to call tradeManager.onAllServicesInitialized() before arbitrationManager / mediationManager / refundManager are started.

TradeManager must be inited before MailboxMessageService
@ripcurlx
Copy link
Contributor

@chimp1984 Could you please have a quick look at these changes, just to be sure we are not missing any hidden sideffect that the original order should have prevented. Thanks!

@ripcurlx
Copy link
Contributor

I also experienced non-reproducable issues with MailboxMessages in the TraderChat. They are stuck in Message saved in receiver's mailbox although it seems in the log they were loaded during startup by the other peer. Unfortunately when I started debugging I wasn't able to reproduce the issue again. Looks also a little bit like a race condition. I'm looking into it in more detail right now.

chimp1984
chimp1984 previously approved these changes Feb 23, 2021
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

ACK

I tested just that use case and it fixes the problem.

The underlying problem is that the order how listeners are registered and the domains are initialized is not sufficiently clearly organized.
SupportManager registers its listener in the constructor, others in onAllServicesInitialized.

Listener are moslty held in HashSets, so the order of execution is not related to the order how they get added, which is semantically correct as listeners should not rely on order of execution, but as we see the domains would require more precise order.

It would require a larger refactoring to get those processes better defined.
I think the suggested fix carries probably low risk but all main use cases specially those involving mailbox messages should be tested well.

The decryptedMailboxMessageWithPubKeys and decryptedDirectMessageWithPubKeys in SupportManager are remaining TODOs from the past mailbox message refactoring efforts. We hold already the data in the MailboxMessageService and should rather use that as our data owner, instead of the way how we listen and keep track of the mailboxMessages in SupportManager. That would be more in line with other use cases and more clear. But all changes in that area carry quite some risk and complexity...

Upon receipt of onAllServicesInitialized, TraderChatManager should
call tryApplyMessages in order to process any pending mailbox
messages stored up while offline.
@ghost
Copy link
Author

ghost commented Feb 24, 2021

Further to @ripcurlx comment about trader chat messages, there was an issue where trader chat messages were not propagated to the GUI upon startup if a password protected wallet was used. TraderChatManager was not processing mailbox messages when initialized via onAllServicesInitialized. Added d3b62b1 to fix. The issue also happens in v1.5.5.

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.

ACK - tested quite a bit with mailbox messages during trading, chat and mediation and everything worked so far for me. I wasn't able to re-produce my chat mailbox issues with this PR until now. I'll keep an eye for this while continuing release testing, but I think this is save to be merged.

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.

Buyer opens mediation while seller is offline - NullPointerException
2 participants