-
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
✨ Add AsyncIPCProvider #2984
✨ Add AsyncIPCProvider #2984
Conversation
Pardon my ignorance to github work flow but is this implemented yet? Can't seem to import AsyncIPCProvider on newest web3 |
@JustAnotherC0der It's not yet. We're still working on it. |
This is ready for review (finally)! I will add docs tomorrow, but wanted to get the code out there for feedback! |
Thanks for your hard work. I will really appreciate this feature. Any plans to make AsyncIPCProvider a PersistentConnectionProvider? I think without this it will be impossible to
This is not a big deal since I can still use filters to get similar functionality. |
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!
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.
Thanks for everyone's contribution here. I think this will be a great option to have. I left some initial comments. Mostly curious about the PersistentConnectionProvider
question.
This code won't work if the client receives multiple responses before it reads the first one. I have coded up a dirty fix for my own use. Perhaps it might be useful as a hint: DavidRomanovizc/web3.py@main...SheldonHolmgren:async_ipc_multiple_resps |
Thank you @SheldonHolmgren! I'll see what I can do. |
All right, I think this is ready again for another look. Thanks @fselmo for the boost! |
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.
I caught one minor doc change we missed but that was all I saw (and I went ahead and pushed it). Thanks for all the work here @DavidRomanovizc, @kclowes, and @SheldonHolmgren! lgtm 👍🏼
@kclowes I'll let you merge this in when you are good with it. Maybe some more squashing if it makes sense too?
- Iron out some issues with rebasing; update async_ipc
- Time subscription tests correctly - Fix lint - Fix doc underline - Remove sleeps in test to test multiple responses coming in quick succession - Handle when multiple responses come in at once - Add test for subscription - timeout -> request_timeout Updates to naming: - Remove ws specific naming from manager methods - _ws_message_listener -> _message_listener - WebsocketConnection -> PersistentConnection - persistent_websocket -> persistent_connection
- Clean up ``AsyncIPCProvider``. Some logging c/p was left in the code. - Remove the need for the ``PersistentSocket`` class. - Move the ``silence_listener_task_exceptions`` setting to the base class, ``PersistentConnectionProvider``. - Use the suggestion put forth in the PR #2984 for parsing messages from the IPC socket more efficiently in case more than one message is received at once.
- Update docs for persistent connection provider refactor Remove sleeps to emulate responses in rapid succession
web3/manager.py
Outdated
|
||
async def _ws_message_stream(self) -> AsyncGenerator[RPCResponse, None]: | ||
async def _message_stream(self) -> AsyncGenerator[RPCResponse, None]: | ||
if not isinstance(self._provider, PersistentConnectionProvider): | ||
raise TypeError( | ||
"Only websocket providers that maintain an open, persistent connection " |
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.
It looks like we're removing references to websockets, and either just using "socket" or "persistent connection". Should that happen here too?
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.
oop, yep. Nice catch!
docs/providers.rst
Outdated
@@ -261,7 +376,8 @@ Persistent Connection Providers | |||
|
|||
.. py:class:: web3.providers.persistent.PersistentConnectionProvider(endpoint_uri: str, request_timeout: float = 50.0, subscription_response_queue_size: int = 500) | |||
|
|||
This is a base provider class, currently inherited by the ``WebsocketProviderV2``. | |||
This is a base provider class, currently inherited by the ``WebsocketProviderV2``, | |||
and the ``AsyncIPCProvider``. | |||
It handles interactions with a persistent connection to a JSON-RPC server. Among | |||
its configuration, it houses all of the | |||
:class:`~web3.providers.websocket.request_processor.RequestProcessor` logic for |
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.
This moved to web3.providers.persistent.request_processor
I believe.
What was wrong?
IPCProvider needs async version
Related to Issue #2973
Closes #1413
How was it fixed?
Add AsyncIPCProvider
Todo:
Cute Animal Picture