This comment is no longer true, and seems like it means there can now be missing notifications. I wonder if a different approach would be to send the loadwallet notification before locking the chain & scanning instead of after?

In commit "Handle no threads serving in MaybeScheduleProcessQueue" (dc62f3c)

I think it would make more sense to add this logic at the bottom of AddToProcessQueue (and avoid calling MaybeScheduleProcessQueue in no thread case) than having it at the top of MaybeScheduleProcessQueue and returning early.

Few reasons:

• Having this logic doesn't seem useful in ProcessQueue, the other place where MaybeScheduleProcessQueue is called
• Adding this code here creates recursion ProcessQueue -> MaybeScheduleProcessQueue -> EmptyQueue -> ProcessQueue
• If code is added here the name MaybeScheduleProcessQueue is now misleading because this is running callbacks immediately, not scheduling them for later

In commit "Handle no threads serving in MaybeScheduleProcessQueue" (201eebf)

Commit message subject is out of date since MaybeScheduleProcessQueue is no longer changing.

Also I'm surprised by "SyncWithValidationInterfaceQueue is called after
scheduler thread exists, which happen at shutdown" since I would hope wallets are getting unloaded before the scheduler stops, but maybe this is not the case currently. Probably beyond the scope of this PR to address, though

since I would hope wallets are getting unloaded before the scheduler stops

Not currently the case.

Probably beyond the scope of this PR to address, though

Agree, I've tried and it caused some other failures.

Commit message subject is out of date since MaybeScheduleProcessQueue is no longer changing.

Ops need to update.

In commit "Handle no threads serving in MaybeScheduleProcessQueue" (201eebf)

If we can make this code:

if (m_pscheduler->AreThreadsServicingQueue()) [
    MaybeScheduleProcessQueue();
} else {
    EmptyQueue();
}

it would be more obvious what's going on here.

Otherwise if the unconditional MaybeScheduleProcessQueue call is needed it would be good have a comment saying what it's needed for.

Should it be inside EmptyQueue

Should it be inside EmptyQueue

I wouldn't change emptyqueue because it's called other places which don't need this logic, and I can't see how the behavior would fit in there (how you could coherently describe it or change the emptyqueue name to reflect it)

Changed. However if the scheduler thread is stopped after AreThreadsServicingQueue then the function won't be called right?

Test wallet_multiwallet fails due to this code path, see my changes in EmptyQueue, you should schedule process queue when event queue is not empty, the logic is reversed there.

It makes much more sense compare to current version

void SingleThreadedSchedulerClient::EmptyQueue() {
    if (!m_pscheduler->AreThreadsServicingQueue())
        return;
    auto pendingCallbacks = [this]() -> bool {
        LOCK(m_cs_callbacks_pending);
        return !m_callbacks_pending.empty();
    };
    bool should_continue = pendingCallbacks();
    while (should_continue) {
        ProcessQueue();
        should_continue = pendingCallbacks();
    }
}

1. You don't need to do stupid things, like here one,
   if (!m_pscheduler->AreThreadsServicingQueue())
2. You don't need to check against is there pending callbacks it's done there, so if you don't have pending ones you don't need to enter eventually endless waiting.

Iv'e been trying to reproduce appveyor error wihtout any luck.

1. You don't need to do stupid things, like here one,
   if (!m_pscheduler->AreThreadsServicingQueue())
2. You don't need to check against is there pending callbacks it's done there, so if you don't have pending ones you don't need to enter eventually endless waiting.

@bvbfan, I'm confused by what you are saying here. You seem to be saying you don't need two things, but your code sample is doing both those things. Are you saying that for some reason it is better to do (1) in EmptyQueue instead of AddToProcessQueue? If so, what is the reason you think this? To me it seems better to do (1) in AddToProcessQueue instead of EmptyQueue, because AddToProcessQueue has to be aware of thread state while EmptyQueue does not.

I'm even more confused what you are saying about (2) because you're saying we don't need a check for pending callbacks but your code is actually adding an extra check (before the first loop iteration).

More importantly, I can't figure out what this suggestion has to do with wallet_multiwallet.py. If you can clarify, it would be much appreciated. I've also been trying to reproduce the failure there and look for possible causes and haven't been able to figure it out.

I'm confused by what you are saying here. You seem to be saying you don't need two things, but your code sample is doing both those things.

I mean you don't need to do these things outside else, i was not clear. These 2 things should be done here in EmptyQueue

More importantly, I can't figure out what this suggestion has to do with wallet_multiwallet.py

if (m_pscheduler->AreThreadsServicingQueue()) [
    MaybeScheduleProcessQueue();
} else {
    EmptyQueue();
}

This code is wrong, you try to process / flush events but it does not do that. Let analyse current code

void SingleThreadedSchedulerClient::EmptyQueue() {
assert(!m_pscheduler->AreThreadsServicingQueue()); // <- assert (you can avoid that)
   bool should_continue = true;
   while (should_continue) {
       ProcessQueue(); // <--- here is the bigger mistake, it tries to process events before it's checked that has pending callbacks, you can enter this and wait to infinity (since app is shutting down) it should be checked for pendings before process queue
       LOCK(m_cs_callbacks_pending);
       should_continue = !m_callbacks_pending.empty();
   }

       ProcessQueue(); // <--- here is the bigger mistake, it tries to process events before it's checked that has pending callbacks, you can enter this and wait to infinity (since app is shutting down) it should be checked for pendings before process queue

Explain this more? ProcessQueue doesn't wait for callbacks if there aren't any, it just runs a callbacks if there is one.

Iv'e been trying to reproduce appveyor error wihtout any luck.

I finally reproduced the problem cutting down the test and running in a loop with --usecli. Looking at the stack trace I see unloadwallet RPC thread waiting for scheduler thread MaybeCompactWalletDB call, which is stuck in ReleaseWallet calling SyncWithValidationInterfaceQueue, which is stuck because SyncWithValidationInterfaceQueue will always hang if called from the scheduler thread. This is basically the same deadlock that was in fanquake's backtrace from #16307 (comment), which suggests that 2a42fc1 and 2097e35 aren't going to work as fixes for this problem in their current form. Maybe more can be done to get them working but an alternate approach like 06d2b53 (branch, comment) might be easier at this point

In commit "Handle no threads serving in MaybeScheduleProcessQueue" (03cd591)

Was experimenting and wrote a small test for this change:

// Ensure scheduled tasks run even when there are no scheduler threads
BOOST_AUTO_TEST_CASE(singlethreadedscheduler_nothreads)
{
    CScheduler scheduler;
    SingleThreadedSchedulerClient client(&scheduler);
    bool task_ran = false;
    client.AddToProcessQueue([&] { task_ran = true; });
    BOOST_CHECK(task_ran);
}

Could add this to scheduler_tests.cpp. It fails without this commit and passes with it.