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

FastHttpUser improvements (including a rename of parameter "url" to "path") #2083

Merged
merged 5 commits into from
May 3, 2022

Conversation

mgor
Copy link
Contributor

@mgor mgor commented Apr 29, 2022

Sorry for the long "intro", which might seem irrelevant at first... but bear with me :)

In "my" project I had a case where I wanted to create a group of [async] requests, by running them in their own greenlet.
Normally it uses the requests client via HttpUser, but the way requests is using the ConnectionPool from urllib3 is not thread-safe[0], which caused the requests to run more or less sequential instead.

This lead me to using a FastHttpSession for each async request, which worked great for that part. But instead ran into some typing issues, e.g.:

class FastResponse(CompatResponse):
  headers = None
...

Since geventhttpclient isn't typed, this causes mypy to think that headers is always None, causing it to get type Never, making it hard to actually use these attributes without using type: ignore-comments.

The other problem inconvenience I had, was that locust.contrib.fasthttp.FastResponse was so different from requests.models.Response, causing "my" code that handles the response (for logging purposes) to get a bunch of special cases depending on what type of response it was. I've tried my best to make it easier to use FastHttpSession as a drop-in replacement for HttpSession.

One thing that I haven't changed, is that locust.clients.HttpSession.request has url as argument, where as locust.contrib.fasthttp.FastHttpSession.request has path. I would love to change that... but thought that it might be a too big breaking change(?).

Also, once again I had problems with test_runners::test_distributed_shape_with_fixed_users. I increased it in a previous PR from 0.5 to 1.0, but seems like it sometimes is not enough, so now I changed so master.status is polled in a gevent.Timeout with 3 seconds, instead of having a static sleep so the test shouldn't fail when it sometimes takes a little longer. That check would take at minimum 0.1 seconds and at maximum 3 seconds when it's actually is a problem.

[0] psf/requests#1871

typing and more requests.model.Response like.
poll master.state up to 1.5 second timeout, instead of a static sleep.
create custom CompatRequest and use as it as request type.
@mgor
Copy link
Contributor Author

mgor commented Apr 29, 2022

ehm, really strange that two test cases in test_dispatch.py failed, only on python 3.7... i couldn't see that it would be related to any of the changes?

@heyman
Copy link
Member

heyman commented May 2, 2022

Nice! This looks like an improvement to me.

One thing that I haven't changed, is that locust.clients.HttpSession.request has url as argument, where as locust.contrib.fasthttp.FastHttpSession.request has path. I would love to change that... but thought that it might be a too big breaking change(?).

I agree that it would be a nice change. As long as we don't do it in a patch release, and clearly document it in the Docs' Changelog, I think it's worth doing. It should be a simple fix changingpath to url for anyone using request() with keyword arguments (code calling request() without keyword args for method and path should be compatible).

@cyberw cyberw changed the title fasthttp improvements FastHttpUser improvements (including a rename of parameter "url" to "path") May 3, 2022
@cyberw cyberw merged commit 63e2c7d into locustio:master May 3, 2022
@cyberw
Copy link
Collaborator

cyberw commented Jun 23, 2022

This is really odd. The timeout keyword stopped working now, but I cant understand why. It seems to have broken with the new release of gevenhttpclient, but I see no relevant changes there. https://github.com/locustio/locust/runs/6991527717?check_suite_focus=true

@mgor
Copy link
Contributor Author

mgor commented Jun 23, 2022

Strange. I can have a look on Monday, on my way to our midsommer destination right now.

@cyberw
Copy link
Collaborator

cyberw commented Jun 23, 2022

Awesome, thanks. Glad midsommar!

@cyberw
Copy link
Collaborator

cyberw commented Jun 27, 2022

As far as I can tell, geventhttpclient never supported timeout as a parameter, but it just happened to not complain about it earlier (just ignoring it). I will remove its use from our tests.

@mgor mgor deleted the feature/fasthttp_improvements branch June 27, 2022 19:56
@mgor
Copy link
Contributor Author

mgor commented Jun 27, 2022

oh crap, I totally forgot to have a look at this today as promised (came back to a full calendar).
I can take a quick look now.

@cyberw
Copy link
Collaborator

cyberw commented Jun 27, 2022

I think I have it figured out, no worries :)

@mgor
Copy link
Contributor Author

mgor commented Jun 27, 2022

seems like this is the commit that caused timeout to slip-through in urlopen to _make_request.
**kwargs was added to the call to _make_request, but that method does not have a "catch-all" **kwargs in the arguments.

geventhttpclient/geventhttpclient@20f85d7#diff-89faf154761b75cf1fc552eb81207dfe2bc3adc61329e93dea127ca65b458f58R430

and I guess the timeout parameter was "inherited" from the test_http tests:
https://github.com/locustio/locust/blob/master/locust/test/test_http.py#L28

@cyberw
Copy link
Collaborator

cyberw commented Jun 27, 2022

👍

Hmm... I guess at the very least I should bump the minor version of geventhttpclient (and remove the 1.5.4 release) as that this is much more than a patch version change. Or should we do something else?

@cyberw
Copy link
Collaborator

cyberw commented Jun 27, 2022

@gwik was it intentional that geventhttpclient should ignore extra parameters?

I dont have the whole "origin story" ;)

@cyberw
Copy link
Collaborator

cyberw commented Jun 27, 2022

I have changed geventhttpclient so that unknown arguments are not forwarded, thus restoring full backwards compatibility, even for strange use cases (released in 1.5.5, yanked 1.5.4)

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

3 participants