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.

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 flag to is_connected to allow user to see why provider connection failed #2912

Merged
merged 9 commits into from
Apr 24, 2023

Conversation

pacrob
Copy link
Contributor

@pacrob pacrob commented Apr 11, 2023

What was wrong?

w3.is_connected() just returns True or False. It would be helpful to know why the connection failed if it is False.
Closes #2910

How was it fixed?

Added a raise_if_false flag to the is_connected method and a new ProviderConnectionError which inherits from Web3Exception.

While adding tests, did a little cleanup on other provider testing.

Todo:

Cute Animal Picture

image

@pacrob pacrob force-pushed the improve-is_connected-failure branch 2 times, most recently from fc48a9c to 78c5d0d Compare April 11, 2023 21:12
@pacrob pacrob changed the title [WIP] add raise_if_false flag to is_connected to allow user to see why provider connection failed [WIP] add flag to is_connected to allow user to see why provider connection failed Apr 12, 2023
@pacrob pacrob force-pushed the improve-is_connected-failure branch 7 times, most recently from 0bec883 to 72dcab4 Compare April 20, 2023 15:56
@pacrob pacrob changed the title [WIP] add flag to is_connected to allow user to see why provider connection failed add flag to is_connected to allow user to see why provider connection failed Apr 20, 2023
@pacrob pacrob force-pushed the improve-is_connected-failure branch from 72dcab4 to 2a59ab4 Compare April 20, 2023 16:20
@pacrob pacrob force-pushed the improve-is_connected-failure branch from 2a59ab4 to 691a44e Compare April 20, 2023 16:30
@pacrob pacrob requested review from fselmo and kclowes April 20, 2023 17:03
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.

Looks good! I had a naming nit, and one question about the cache size test, but feel free to take or leave!

@@ -101,13 +101,17 @@ setting the middlewares the provider should use.
the JSON-RPC method being called.


.. py:method:: BaseProvider.is_connected()
.. py:method:: BaseProvider.is_connected(raise_if_false=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm finding raise_if_false to be sort of hard to reason about. Maybe we could call this argument show_traceback or show_stack_trace or something along those lines instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like show_traceback. Changed.


This function should return ``True`` or ``False`` depending on whether the
provider should be considered *connected*. For example, an IPC socket
based provider should return ``True`` if the socket is open and ``False``
if the socket is closed.

If set to ``True``, the optional ``raise_if_false`` boolean should raise a
Copy link
Collaborator

Choose a reason for hiding this comment

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

😄

Suggested change
If set to ``True``, the optional ``raise_if_false`` boolean should raise a
If set to ``True``, the optional ``raise_if_false`` boolean will raise a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -83,5 +90,5 @@ async def test_user_provided_session() -> None:
session = ClientSession()
provider = AsyncHTTPProvider(endpoint_uri=URI)
cached_session = await provider.cache_async_session(session)
assert len(request._async_session_cache) == 1
assert len(request._async_session_cache) == 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do these changes increase the cache size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The is_connecteds I added were added to the cache. Updated to clear the cache at the end of the test.

@pacrob pacrob force-pushed the improve-is_connected-failure branch 2 times, most recently from 2c115e4 to 19faa57 Compare April 24, 2023 17:10
@pacrob pacrob force-pushed the improve-is_connected-failure branch from 19faa57 to 06d6ee2 Compare April 24, 2023 17:17
@pacrob pacrob merged commit 71ae249 into ethereum:master Apr 24, 2023
@pacrob pacrob deleted the improve-is_connected-failure branch April 24, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show failure reason for BaseProvider.is_connected()
2 participants