check _request_information_cache value is a dict before .get-ing from it

[pacrob]

What was wrong?

Mixing batching and non-batching calls causing issues.

Closes #3642

How was it fixed?

• Check that the value from cache is a dict before getting from it.
• Add try/finally blocks (via a decorator) to all make_batch_request functions to make sure _is_batching is True when called and reset to False afterwards.

Todo:

• Clean up commit history
• Add or update documentation related to these changes
• Add entry to the release notes

Cute Animal Picture

[Copilot]

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

Files not reviewed (1)

• newsfragments/3642.bugfix.rst: Language not supported

Comments suppressed due to low confidence (1)

web3/providers/persistent/persistent.py:336

• The indentation of the if-statement checking for 'error' is inconsistent with the surrounding block, which may cause unexpected behavior. Consider aligning it properly within the 'if isinstance(response, dict):' block.

                      if "error" in response and request is None:

[pacrob]

@fselmo I've added the try/finally as suggested. Good as is, or is there a reason to wrap just the sending of the request itself?

I applied it across all instances of make_batch_request for consistency. Is there a reason to only apply it to the persistent provider?

[fselmo]

Looks good. I wanted to suggest the decorator as I think that could DRY things up and reduce common surface area to one place.

Is there a reason to only apply it to the persistent provider?

This should be applied to all providers that support batching (as you did) 👍🏼

[fselmo]

Code looks good, just some refactor ideas to think about. It would also be good to have a simple test that actually checks this state. Given that we also do the dict check (the first commit in this PR), I don't think any tests actually fail without this implementation and then pass after it is implemented (correct me if I'm wrong here).

It doesn't have to be a super complex test but something that tracks this change in case we ever meddle with it and accidentally turn it off would be ideal.

[pacrob]

something that tracks this change

Tests added for all providers (except legacy websocket), and verified a failure when the decorator is commented out.

I decided to not test legacy websocket because even when I got its test to pass, I couldn't get rid of the unclosed event loop warnings, plus its being due to be removed sooner than later. Let me know if you'd like me to take another go at it.

[fselmo]

What do you think of a test that checks both on and off behavior? Something like this:

@patch(
    "web3._utils.http_session_manager.HTTPSessionManager.make_post_request",
    new_callable=Mock,
)
def test_http_provider_is_batching_when_make_batch_request(mock_post):
    def assert_is_batching_and_return_response(*_args, **_kwargs) -> bytes:
        assert provider._is_batching
        return b'{"jsonrpc":"2.0","id":1,"result":["0x1"]}'

    provider = HTTPProvider()
    assert not provider._is_batching

    mock_post.side_effect = assert_is_batching_and_return_response

    provider.make_batch_request([("eth_blockNumber", [])])
    assert not provider._is_batching

That way we check that it turns on while we are making the post request and that it turns back off after the call. We could add a similar test across all the providers.

[pacrob]

OK, all updated to the new pattern. Thanks!

[fselmo]

lgtm once tests pass 👌🏼 (all good on the legacy ws, I agree on the deprecation reasoning)