♻️ 💥 Refactor EventBus notify method

[ff137]

In summary:
💥 Breaking change: EventBus.notify() is now non-blocking, and all subscribed coroutines will run as background tasks.

Previously, subscribed handlers were run sequentially within the notify call, and all had to complete for notify to return.

---

This means that some of the more complex handlers, e.g. revocation-registry setup, which included chained notifications, can now complete earlier, while the remaining notifications queue off asynchronously. This makes it a major performance improvement to a core component. But comes with the "breaking change" caveat, because any flow that immediately expected to issue a credential using a revocable cred-def that was just created, will now need to wait a bit.

That's the only scenario that broke with this change: timing issues with POST /credential-definition returning earlier, while resources were created asynchronously. So, to cater for this "backward compatibility" issue:

• ✨ Added a wait_for_active_revocation_registry utility method, which can be configured (skipped if desired) using a wait_for_revocation_setup boolean field in both POST /credential-definition endpoints.

• 🔧 Added two new env vars:

  • REVOCATION_REGISTRY_CREATION_TIMEOUT, to configure revocation-setup timeouts (default 60s).
  • MAX_ACTIVE_EVENT_BUS_TASKS, to configure the queue size for the new event bus task queue (default 50).

• 🐛 Exception handler added to AnonCredsRevocation._create_credential_helper, so that if "No active registry" error occurs, it will continue retrying a few times before raising. This was necessary for the Indy case, where waiting for an active registry record was not enough, as there was still some timing issue there.

• 🎨 Also reorganizes some code (mainly constants), in order to help with cyclic import issues that came up, and fixes some "coroutine not awaited" warnings in tests, which tripped up PR test results.

---

❓ Question: The EventBus.notify method can now actually be made synchronous, because it just adds coroutines to a queue. That would be a secondary breaking change, because plugins or other libraries would no longer "await" the notify method. Maybe that change may as well be included now; maybe it can be done separately?

---

Original PR description:

This PR modifies the EventBus.notify method to now "fire and forget", instead of blocking the thread making the notify call.

This is achieved by moving event processors to background tasks, using the existing TaskQueue implementation.

This is especially helpful for event processors that involve ledger operations, which can take quite long. The function making the notify call never expects any result - it just wants to fire off the message and move on - and this PR will now allow that.

---

Side note: The refactored revocation registry management PR (#3831) introduces more granular events for making the state machine events recoverable. Without this change, the events are chained together. One .notify() continuously activates the next -- so if "notification complete" logs were added right after each .notify call, you will only see the very first notify completing once the inner-most nested notify call was complete. #3831 depends on events being completed as quickly as possible, so they can be monitored / recovered properly

[swcurran]

Shoot — sorry, didn’t see that this was draft. Should have waited for the tests to complete before merging in main. Doh...

[ff137]

Note: the PR tests in acapy_agent/protocols/present_proof/dif/tests/* are taking exceptionally long to complete.
(Not related to these changes.)

Full pytest run usually takes ~3 minutes, and it currently takes around 25 minutes, since about 2 hours ago.
Not sure why, but presumably there's some network load. And it's probably good at some point to mock the network calls for these unit tests, so the network is not a blocker for PRs

[ff137]

The problem with scenario tests is that now the revocation registries are not guaranteed to be ready by the time the tests tries to use them.

I'll see what can be done about this. I presume that creating a revocable cred def would previously wait for events associated with rev-reg-def's to be complete before returning. So that behaviour can maybe be preserved somehow.

[ff137]

EventBus.notify can now be made synchronous, but it would require updates to plugins that call it (would be breaking change if async is removed, since it shouldn't be awaited). So, I just added a todo

[ff137]

Finally, took a long time to fix the tests, but now it's actually ready for review!

[jamshale]

This is very nice 👍

I also think we should change it to not be asynchronous in a separate task later. Maybe as a targeted release. Just to make the plugin updates more isolated and if there is any potential problems with the work around this it will be easier to find.

We also want to get the Kanon profile stuff completed. I'm not sure how many merge conflicts this will cause with it, but if it's ready now, it might be better to merge that first.

[ff137]

I also think we should change it to not be asynchronous in a separate task later. Maybe as a targeted release. Just to make the plugin updates more isolated and if there is any potential problems with the work around this it will be easier to find.

We also want to get the Kanon profile stuff completed. I'm not sure how many merge conflicts this will cause with it, but if it's ready now, it might be better to merge that first.

Agreed on both 👍
Good to narrow the scope and do async def -> def change separately
And I think good to merge the Kanon stuff first. It'll be a smaller headache for me to fix merge conflicts here, than yet again making them fix merge conflicts there. So I'm happy to wait for that merge first.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
4 Accepted issues

Measures
0 Security Hotspots
90.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[esune]

Did a pass, changes look great - much cleaner code in many sections. Will look out for a rebase after the other PR is merged for final review