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

Benchmark Test Fix #2235

Closed
wants to merge 2 commits into from
Closed

Conversation

dbfreem
Copy link
Contributor

@dbfreem dbfreem commented Dec 3, 2021

What was wrong?

Related to Issue #2234

How was it fixed?

Removed the try catch around the sync and async benchmark methods. This was causing the benchmark test to fail silently.

Todo:

Still need to figure out what is causing the POST request in request.py to timeout.

@dbfreem
Copy link
Contributor Author

dbfreem commented Dec 3, 2021

still going to look at why the POST in request.py is getting a timeout

@dbfreem dbfreem changed the title Benchmark Test Fix #2234 Benchmark Test Fix Dec 3, 2021
@dbfreem
Copy link
Contributor Author

dbfreem commented Dec 5, 2021

@kclowes something I noticed while testing this some more. There is a test Geth that gets spun up as part of the benchmark. After a large number of sendTransaction request are submitted to it, it just stops responding. Even if I send a curl from the terminal to it, it will never respond after it takes about 100 to 200 request. It does respond to just a simple get request but the sendTransacation stops working. It seems to me that this is more of an issue with the test Geth node than web3.py benchmarking itself.

The screenshot below was taken after the TimeOut is received in the benchmark test but before I shut down the debug session, so the node is still running. The request never gets a response. In the second screenshot I got a 200 on a POST request to the node with no body, so it seems this is specific to the node and sendTransaction

image

image

@kclowes
Copy link
Collaborator

kclowes commented Dec 6, 2021

That is super strange. Thanks for looking into it. Maybe that's why we keep getting timeouts on our integration tests too 🤔

@dbfreem
Copy link
Contributor Author

dbfreem commented Dec 8, 2021

@kclowes at this point the above PR would be my recommendations. I can't quit figure out why the request to the node start to timeout after a certain number of request. I changed the number of request that are run in the benchmark test, also I removed the throw catch that was causing the benchmark test to fail silently.

One thing that I did note is that a lot of the timeouts were happening in the async request. I noticed that in the request.py module that the async request are not caching the session like they are on the requests http request.

geth mentions issues can arise from too many sessions. issue

also the aiohttp library recommends reusing sessions

I was thinking of implementing the caching of the session on the aiohttp request in the request.py on a different PR if that makes sense to you.

@kclowes
Copy link
Collaborator

kclowes commented Dec 17, 2021

Thanks for looking into this @dbfreem! I just added a comment to the original issue. I was mistaken, the benchmarking tests are actually running as they should, there are just a bunch of asyncio.exceptions.TimeoutErrors that need to be cleaned up.

@dbfreem
Copy link
Contributor Author

dbfreem commented Dec 17, 2021

I added some comments on that issue with some of my findings.

@dbfreem dbfreem closed this Jan 26, 2022
@dbfreem dbfreem deleted the feature/benchmark branch July 9, 2022 10:16
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.

None yet

2 participants