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.

Sign up for GitHub

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

Jump to bottom

Subscription management implementation #3554

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Subscription management implementation #3554

wants to merge 11 commits into from

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Dec 9, 2024 •
edited
Loading

What was wrong?

Related to Issue #3397

Working with many subscriptions is not ideal at the moment. In web3.py v7, we added significant support for eth_subscribe via the new PersistentConnectionProvider implementation. Though we've added support with parity to how websockets works, this still only left us with a raw API to handle messages as they come through the socket. We have JSON-RPC specifications of how subscriptions should be formed and the client returns a unique id per subscription. Since PersistentConenctionProvider classes are fully asynchronous, as they should be, we should implement an API that can leverage all of this in order to handle subscriptions asynchronously as they come in.

How was it fixed?

  • Implement a subscription manager class to manage all subscriptions under-the-hood. This manager can be accessed directly to deal with subscriptions as objects, rather than free-floating ids that are tracked. Each EthSubscription class can, and should be encouraged to, have a handler function passed to it so that it can properly handle the subscription as it comes in, rather than having the burden of listening and parsing based on message id be on the user.
  • w3.eth.subscribe can now also accept a handler kwarg since all subscriptions are managed by the manager under-the-hood. It can also accept an event for "logs" subscriptions so that it may be easily parsed by the handler.
  • Subscriptions can have descriptive labels for clearer log messages, if desired, instead of relying on an abstract subscription id to parse logs.
  • The manager counts how many total handler calls have been issued and each subscription counts how many times its handler has been called.

In a separate PR we should add a significant docs overhaul for clearer communication on PersistentConnectionProvider usage as well as subscription management usage

Todo:

  • Clean up commit history
  • Early documentation before a significant docs overhaul to highlight this usage more appropriately
  • Add tests
  • Add entry to the release notes

Cute Animal Picture

1000029633

@fselmo fselmo changed the title Subscription management rnd Subscription management implementation Dec 9, 2024
@fselmo fselmo force-pushed the subscription-management-rnd branch 13 times, most recently from 3d06c95 to 828b8b2 Compare December 13, 2024 00:05
@fselmo
Copy link
Collaborator Author

fselmo commented Dec 13, 2024

This is ready for a review :)

@fselmo fselmo force-pushed the subscription-management-rnd branch 2 times, most recently from 23d7f11 to 8644827 Compare December 13, 2024 02:04
@fselmo fselmo marked this pull request as ready for review December 13, 2024 02:16
@fselmo fselmo self-assigned this Dec 16, 2024
@fselmo fselmo force-pushed the subscription-management-rnd branch 3 times, most recently from 3f355ec to 21f396d Compare December 17, 2024 17:37
@fselmo
Copy link
Collaborator Author

fselmo commented Dec 17, 2024

I moved to a context-based args management for subscription handlers in order to facilitate custom args to be used within handlers. This is back on a freeze for reviews.

@fselmo fselmo force-pushed the subscription-management-rnd branch from 21f396d to 3918d63 Compare December 17, 2024 17:52
reedsa
reedsa reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

Left a few ideas for consideration but didn't see anything that sticks out as a showstopper. Nice work!

web3/_utils/module_testing/module_testing_utils.py Show resolved Hide resolved
web3/providers/persistent/subscription_manager.py Show resolved Hide resolved
web3/providers/persistent/subscription_manager.py Outdated Show resolved Hide resolved
web3/providers/persistent/subscription_manager.py Outdated Show resolved Hide resolved
fselmo added a commit to fselmo/web3.py that referenced this pull request Dec 17, 2024
@fselmo
Changes from comments on ethereum#3554; update docs.
25c7f93
fselmo added a commit to fselmo/web3.py that referenced this pull request Dec 19, 2024
@fselmo
Address remaining comments from PR ethereum#3554:
e10f028 
- ``sx`` -> ``sub``.
- Some clarifying changes on type hints and arg names in tests.
- Docs update to clarify handlers. This whole area of the docs will get
  a lot of attention and refactoring in a future PR. But at least the
  current docs are more descriptive.
@fselmo fselmo force-pushed the subscription-management-rnd branch from 4d390dd to e10f028 Compare December 19, 2024 21:32
@fselmo
Copy link
Collaborator Author

fselmo commented Dec 19, 2024 •
edited
Loading

This doesn't break the existing API, right?

@kclowes, always good to ask 😅. The relevant API changes only introduce more customization. This shouldn't break the API. And if no handlers are passed in (new feature), you can still listen to free-flowing messages directly from the socket.

The typing in the socket stream was already Optional meaning a None could technically be returned as a "message". If a message has a handler a None is returned so that the iterator can move on to the next iteration. This is the only friction I see this PR introducing. I didn't see a clean way to move a users async for message in socket to the next message, if it was handled in a handler, without yielding a None but perhaps we can investigate that a bit more.

Thoughts on that?

@fselmo
Address remaining comments from PR ethereum#3554:
2d4958a 
- ``sx`` -> ``sub``.
- Some clarifying changes on type hints and arg names in tests.
- Docs update to clarify handlers. This whole area of the docs will get
  a lot of attention and refactoring in a future PR. But at least the
  current docs are more descriptive.
@fselmo fselmo force-pushed the subscription-management-rnd branch from e10f028 to 2d4958a Compare December 19, 2024 21:54
kclowes
kclowes approved these changes Dec 20, 2024
View reviewed changes
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

I like this! I didn't have time to go through with a fine tooth comb, but if you feel good about it and the tests, don't let me block your merge 👍

@fselmo fselmo force-pushed the subscription-management-rnd branch 5 times, most recently from c33d49c to 39f7b9f Compare December 21, 2024 01:05
@fselmo
Copy link
Collaborator Author

fselmo commented Dec 21, 2024 •
edited
Loading

I like this! I didn't have time to go through with a fine tooth comb, but if you feel good about it and the tests, don't let me block your merge 👍

It has been pretty battle tested but it's causing some irrelevant flaky replace transaction tests to fail pretty consistently, just on CI. I checked against main and it is specific to this branch. I'll wait a little longer to debug it and try to figure out what's causing it. The frustrating thing is that locally it isn't reproducible (these flaky tests haven't been reproducible locally all along). So I might push a commit on top of these until I can sniff it out and try to assess, roll the commit back, and handle appropriately.

My suspicion is it's not directly related to this PR. Ultimately those tests need to be properly fixed, so that might be what I end up doing in a separate PR.

Either way I'll be sure to preserve the current state of the new code (no force pushing on top of the previous commits) so that nothing that you all have reviewed changes.

@fselmo fselmo force-pushed the subscription-management-rnd branch 2 times, most recently from 7772a9b to d23e800 Compare December 21, 2024 02:29
@fselmo fselmo force-pushed the subscription-management-rnd branch from d23e800 to 461dbd2 Compare December 28, 2024 21:17
@fselmo fselmo force-pushed the subscription-management-rnd branch 8 times, most recently from 04ab8f4 to 6707be0 Compare January 6, 2025 20:13
fselmo added 3 commits January 6, 2025 13:26
@fselmo
Separate handler subscriptions into their own queue:
d665711 
- Make use of a ``TaskReliant`` queue that is specific for handler
  subscriptions. Put a ``SubscriptionProcessingFinished`` exceptions
  in the queue to signal when all handler subscriptions have been
  unsubscribed from.

- Add testing around running the ``handler_subscriptions`` method while
  listening for un-handled subscriptions via the socket.

- Create a container to manage all subscriptions, since we store subs
  in more than one way: by ``id``, by ``label``, etc...

- The ``SubscriptionContainer`` class has an object reference, so we changes
  share this with the provider's ``RequestProcessor`` class. The
  request_processor can use the container to make decisions about which queue
  to store the subscription in, based on the presence or absence of a
  ``handler``.
@fselmo
Lower request timeout for tests. No test should need more than 10s.
d88d6e7
@fselmo
Properly close tasks related to each test before ending tests:
a95267b 
- Properly cancel and close all tasks for persistent connection provider tests
- Clear caches as a cleanup for all persistent connection provider tests
@fselmo fselmo force-pushed the subscription-management-rnd branch 2 times, most recently from db7049c to 6b820cb Compare January 7, 2025 22:07
@fselmo
[Persistent Connection Providers] Improve request info management:
9a2c699 
- Instead of guessing what the request `id` will be and having to bump
  this cache key for internal requests, assign a unique cache key to
  each request. Save these cache keys as a list that can house all cache
  keys for a request and its internal requests.

- Swap this cache key for a cache key generated with the request `id`
  only after the request `id` is already known. This will prevent
  any guesswork. This prevents calls being nmade from other tasks from
  interfering with the request `id` assignment and accidentally
  bumping the cache key for a request while it is still being processed.
@fselmo fselmo force-pushed the subscription-management-rnd branch from 6b820cb to 9a2c699 Compare January 7, 2025 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kclowes kclowes kclowes approved these changes

@wolovim wolovim wolovim approved these changes

@reedsa reedsa reedsa approved these changes

@pacrob pacrob Awaiting requested review from pacrob

Assignees

@fselmo fselmo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fselmo @reedsa @wolovim @kclowes