Implement ThreadPoolExecutor publisher pattern for EventBridge (WIP)

[josefdlange]

Motivation

The PutEvents method on the Event Bridge implementation blocks on waiting for targets to process events. This can make for long chains of dependent event emissions that take forever to resolve the initial PutEvents or even involve retries upstream, creating a sort of thundering herd issue. See issue #10187 for the origin of this issue and where this PR comes from.

Changes

This implements a Publisher class which maintains a ThreadPoolExecutor. This class is used both by the Provider for Event Bridge and passed into a callable PutEventsHandler class which takes the place of the previous handler function -- both share the publisher instance owned by the Provider class which controls thread pool shutdown behavior via its lifecycle methods.

Testing

Testing incoming. I wanted to get something up to get some other eyeballs on it as I am not terribly familiar with the LocalStack codebase and style.

TODO

What's left to do:

• Test, in the general hand-wavy way to ensure the change even accomplishes my goal
• Test, in the sense that the new code has ample test coverage and follows through on the ideal that all PRs should increase test coverage.

Other notes

• It's plausible that this publisher wrapper for send_event_to_target can be leveraged in other places where send_event_to_target is used, but I want to keep this tidy for the moment, plus I was told in Slack that with the incoming implementation of EB Pipes stuff that this code change may be made obsolete sooner than later.
• I propose that this be labeled as the most minor of changes. No interface behavior changes, no new "feature" per se, and certainly no breaking changes. Unless you're relying on the event processing to block (which would be a discrepancy from AWS behavior...)

[localstack-bot]

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

[localstack-bot]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[josefdlange]

I have read the CLA Document and I hereby sign the CLA

[josefdlange]

The diff, especially around here, is a little messy-looking but it's mainly creating class PutEventsHandler that is callable, the __call__ method being the same implementation as events_handler_put_events. This class also absorbs some functions into internal methods, mainly because they're only used and needed for the handler's sake. Implementation details should remain relatively the same, save for the executor-publisher being involved instead of directly calling send_event_to_target

[josefdlange]

Can I get a little assistance with the failing tests here? The bootstrap fail I cannot reproduce locally.

[dfangl]

Hi @josefdlange !
You are patching a method with an object. An object which implements __call__, but still an object. Moto inspects the responses methods before invoking them, and it has to be a function.

The lines making the check can be found here: https://github.com/getmoto/moto/blob/f9add957c9f8311f7e1aa927de7a83f2cb5adce2/moto/core/responses.py#L570-L573
The method used can be found here: https://github.com/getmoto/moto/blob/f9add957c9f8311f7e1aa927de7a83f2cb5adce2/moto/core/utils.py#L66-L68

Even though callable, your object does not pass the inspect.isfunction test. Therefore, put_events is not available as a method, and fails on every call.

I would recommend patching it with a function, and not an object. For the bootstrap tests to fail locally you would have to build the image before, and then execute the tests against that image. The error can be reproduced with other tests as well though, for example test_invoke_lambda_eventbridge

[josefdlange]

@dfangl -- thanks! I really thought I could sneak that one by moto and none would be the wiser! Okay, functionifying....

[dfangl]

Hi @josefdlange !
Sorry for the late answer!
There are still some failing tests in our CI runs. They seem to not be aws validated, so the test failure might very well be an issue with the test more than with the implementation.

Still, we need to fix them beforehand. Could you take a stab into validating the test against AWS? You can find a guide on how to do it here: https://docs.localstack.cloud/contributing/integration-tests/#running-integration-tests-against-aws

If you do not have an AWS account available, please just tell me, and I can fix those tests for you :)

[josefdlange]

@dfangl I've been on vacation the last couple weeks and otherwise a little busy 😓 I'll look into this sometime soon!