-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Websockets v2 update #3096
Merged
Merged
Websockets v2 update #3096
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fselmo
added a commit
to fselmo/web3.py
that referenced
this pull request
Sep 14, 2023
fselmo
force-pushed
the
websockets-v2-update
branch
from
September 14, 2023 20:33
af37d88
to
9ccca62
Compare
fselmo
added a commit
to fselmo/web3.py
that referenced
this pull request
Sep 14, 2023
fselmo
force-pushed
the
websockets-v2-update
branch
from
September 14, 2023 20:43
9ccca62
to
08f92b0
Compare
- Create a request processor class for websocket connections with two caches. One cache for storing request information to process a response with the correct formatters and middleware asynchronously (already existed but moved into this new class). The other cache was added to make sure that a request receives the answer it is asking for by matching the request `id` with the response `id`. Any responses received between then are cached in an OrderedDict / SimpleCache FIFO queue and processed in order before the next response. - Some major refactoring and organization would still be ideal. This was a rough dump of ideas to get it working well. - Add `yParity` to TRANSACTION_RESULT_FORMATTERS as that led to some issues with formatting. Perhaps `either_set_is_a_subset` may become too restrictive in the future.
- Add a percentage flag for how much of either set should be present in the other set. - Add `yParity` field to `TRANSACTION_RESULT_FORMATTERS` because this is what caused the whole of transaction formatting to fail since it existed in one set and not the other. - Look for a 90% match between a subscription response and its formatters, rather than requiring either to be a full subset of the other.
fselmo
added a commit
to fselmo/web3.py
that referenced
this pull request
Sep 14, 2023
fselmo
force-pushed
the
websockets-v2-update
branch
from
September 14, 2023 20:53
30dd04f
to
3d991dc
Compare
reedsa
reviewed
Sep 14, 2023
pacrob
reviewed
Sep 14, 2023
fselmo
added a commit
to fselmo/web3.py
that referenced
this pull request
Sep 14, 2023
- Return the request ``id`` from the websocket_v2 provider so we know what response to match it to. This is a bit hacky for now because it will require a whole refactor of what the method ``make_request`` returns. This is an attempt to just get things working before said refactor.
- Move logic for matching response to the request into the provider class. - Move ThreadPoolExecutor and cache locking into provider as well (from the _PersistentConnectionWeb3 class). This shouldn've been the case from the start. - We now do return the appropriate RPCResponse for the request within the ``make_request()`` method and so we don't have to work through the typing differences anymore. - Fix async middleware that return responses to actually return the response and pass it to the next middleware so we get a response object through the pipe.
- Internalize the direct websocket connection on the provider to avoid direct usage of the websocket library which may break expectated results: `provider.ws` -> `provider._ws` - Create a public API on the `_PersistentConnectionWeb3` class via the `WebsocketConnection` class, with a useful UX for interacting with the websocket connection and getting expected results, and attach this to the `_PersistentConnectionWeb3` instance as `w3.ws`.
fselmo
added a commit
to fselmo/web3.py
that referenced
this pull request
Sep 14, 2023
fselmo
force-pushed
the
websockets-v2-update
branch
from
September 14, 2023 23:38
f20a011
to
38b57b0
Compare
Nothing blocking here, I haven't yet tested though. Looks great, love the separate commits and clean messages 💯 |
reedsa
approved these changes
Sep 14, 2023
fselmo
added a commit
to fselmo/web3.py
that referenced
this pull request
Sep 15, 2023
fselmo
force-pushed
the
websockets-v2-update
branch
from
September 15, 2023 21:58
38b57b0
to
5463ca6
Compare
fselmo
added a commit
to fselmo/web3.py
that referenced
this pull request
Sep 15, 2023
fselmo
force-pushed
the
websockets-v2-update
branch
from
September 15, 2023 22:03
5463ca6
to
ec360a5
Compare
- Return the formatted `params` object for `eth_subscription` messages. This includes the subscription `id` which should remove ambiguity from messages if subscribed to multiple subscriptions.
fselmo
force-pushed
the
websockets-v2-update
branch
from
September 18, 2023 20:04
ec360a5
to
86c225f
Compare
pacrob
approved these changes
Sep 19, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was wrong?
The main reason
WebsocketProviderV2
is in beta is that we have to asynchronously process a response from the original request that was made. This isn't so straightforward with the way websockets work because the socket will give back the next item it needs to, whatever that may be, when we callrecv()
. If a user has subscribed to aneth_subscribe
subscription, say something like"newPendingTransactions"
where it will return many transactions almost nearly instantly as soon as a user has subscribed to it, one (or many) of these subscription messages may return before the original request was made for which a user may be expecting a response. Consider the following:How was it fixed?
Thanks to JSON-RPC 2.0 specs, responses are required to return the
id
of the request that asked for the response. If we keep the requestid
and pass it to the method we use to retrieve and process a response, we can wait until this particular response is received and return that. It should take no longer than acall_timeout
. While we look for the response, we need to store every response that came in before it in a transient cache that will then be emptied on each__anext__()
call before we pull anything from the socket again.RequestProcessor
class that is strapped toPersistentConnectionProvider
classes to process two types of caches now (both transient, in the sense that they only store information until it's needed and that time window shouldn't be too long):RequestInformation
cache that already existed. This cache is responsible for storing a request's information so we can have that context and middleware methods to pipe the answer to the request through once we process it asynchronously.make_request()
method of theWebsocketProviderV2
class. This allows us to actually return a response that matches the requestid
within the same method, keeping types happy throughout the code base.provider.ws
websocket connection (asprovider._ws
) and add a public API class (WebsocketConnection
) for interacting with the websocket on the_PersistentConnectionWeb3
class with more predictable behavior and formatting support, etc.Todo:
provider.ws
internal (provider._ws
) and have any "listen" to ws method that is public look in the raw response cache first and process those responses in FIFO before actually callingws.recv()
again. This will be important to document since a user might want to directly callrecv()
on the websocket connection without knowing there may be cached responses ahead of any new responses.Cute Animal Picture