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 does not pass headers correctly during a retry after a "failure exception" such as timeout #2894

Closed
2 tasks done
soitinj opened this issue Sep 6, 2024 · 5 comments
Labels
bug stale Issue had no activity. Might still be worth fixing, but dont expect someone else to fix it

Comments

@soitinj
Copy link
Contributor

soitinj commented Sep 6, 2024

Prerequisites

Description

Issue: We are testing a system with locust that cannot handle the intended load, and thus the system eventually starts producing timeouts and dropping some connections. During these tests, we started noticing errors where the system was complaining about us sending requests with invalid content-type (text/plain instead of application/json). We noticed that the content-type header was passed twice to the target app, once in Upper-Case and once in lower-case.

Investigation (TLDR at bottom): Here is an example entry from our locust logs:

Failure: POST <address>, code=400 - <cut content> headers: {'API_TOKEN': '<API_TOKEN>', 'Content-Type': 'application/json', 'USER_ID': '<testid>' Accept-Encoding': 'gzip, deflate', 'Accept': 'application/json', 'content-type': 'text/plain; charset=utf-8', 'content-length': 204}

FastHttpSession retries sending requests in _send_request_safe_mode after a FAILURE_EXCEPTION on fasthttp.py row 167, in case the exception has no 'response' attribute:

locust.contrib.fasthttp.py:

        try:
            return self.client.urlopen(url, method=method, **kwargs)
        except FAILURE_EXCEPTIONS as e:
            if hasattr(e, "response"):
                r = e.response
            else:
                req = self.client._make_request(
                    url,
                    method=method,
                    headers=kwargs.get("headers"),
                    payload=kwargs.get("payload"),
                    params=kwargs.get("params"),
                )
                r = ErrorResponse(url=url, request=req)
            r.error = e
            return r

However, the second call uses an internal geventhttpclient UserAgent method, _make_request. After investigating further, the underlying geventhttpclient UserAgent urlopen method takes the headers it receives, and uses a Headers object to update its internal default headers with the given headers (with the given headers containing the correct content-type in our case, application/json):

geventhttpclient.useragent.py:

    def urlopen(
        self,
        url,
        method="GET",
        response_codes=valid_response_codes,
        headers=None,
        payload=None,
        to_string=False,
        debug_stream=None,
        params=None,
        max_retries=None,
        max_redirects=None,
        files=None,
        **kw,
    ):
        """Open a URL, do retries and redirects and verify the status code"""
        # POST or GET parameters can be passed in **kw
        # self.default_headers is a Headers object, not a dict
        req_headers = self.default_headers.copy()
        if headers:
            req_headers.update(headers)
        ...
        req = _make_request(
            url,
            method,
            headers=req_headers,
            payload=payload,
            params=params,
            files=files,
            request_type=self.request_type,
        )

This works fine, since the Headers takes into consideration that we're passing content-type as uppercase, 'Content-Type': 'application/json'. The geventhttpclient Headers object tries to preserve the case of the original header, while still performingHeaders.get case-insensitively. However, the _make_request method does not convert the given dict to a Headers object, and when used directly, we bypass the conversion from dict to a Headers -object. Thus the .get below is done case-sensitively during the retry in locust's FastHttpSession.

geventhttpclient.useragent.py:

def _make_request(
    url, method, headers, payload=None, params=None, files=None, request_type=CompatRequest
):
    # Adjust headers depending on payload content
    content_type = headers.get("content-type", None)
    if files:
        ...
    elif payload:
        if isinstance(payload, dict):
            ...
        elif not content_type and isinstance(payload, str):
            headers["content-type"] = "text/plain; charset=utf-8"
            payload = payload.encode()
            headers["content-length"] = len(payload)
        elif not content_type:
            ...

    return request_type(url, method=method, headers=headers, payload=payload, params=params)

Since the dict.get cannot find the upper-case 'Content-Type', the code ends in the elif not content_type and isinstance(payload, str): block (cause the payload content has been converted to str earlier in locust). After this, headers will contain a lowercase 'content-type': 'text/plain; charset=utf-8', added by the above block of code. This was a duplicate 'content-type' header in our case, and our system picked up that one instead of the correct 'Content-Type': 'application/json'

TLDR: The problem can be solved by passing the headers to the _make_request as a Headers object. I think the problem was introduced by this geventhttpclient commit, though I think that the underlying issue is how locust utilizes geventhttpclient.

I have a commit in a fork of locust which seems to solve the problem (for some reason the python 3.10 tests failed). However, the fix feels like a bit of a hack. It may be better to make the commit to geventhttpclient.UserAgent._make_request, since that method only exists because of locust. It would be even better if locust did not depend on the internals of geventhttpclient, as discussed in this issue.

Command line

locust -f mylocustfile.py -t 10s --headless

Locustfile contents

'''
Any locustfile using FastHttpUser should be able to trigger the issue,
as long as the receiving endpoint expects json,
the headers given to FastHttpSession contain 'Content-Type':  'application/json',
and the requests results in one of the following exceptions:
'''

FAILURE_EXCEPTIONS = (
    ConnectionError,
    ConnectionRefusedError,
    ConnectionResetError,
    socket.error,
    SSLError,
    Timeout,
    HTTPConnectionClosed,
)

Python version

3.11.2

Locust version

2.35.1

Operating system

Ubuntu 20.04

@soitinj soitinj added the bug label Sep 6, 2024
@cyberw
Copy link
Collaborator

cyberw commented Sep 6, 2024

Thanks for the deep analysis! We have a different ticket open for using geventhttpclient’s UserAgent #2681

Do you think that would that solve this issue?

@soitinj
Copy link
Contributor Author

soitinj commented Sep 6, 2024

I might have been too hasty in my original analysis. geventhttpclient's UserAgent does not send a request when calling _make_request, it only builds one. However, I think the _make_request is still responsible for adding a lowercase 'content-type': 'text/plain; text/plain; charset=utf-8', since we only use json, and we pass Content-Type using capital letters, and _make_request suspiciously enough adds it lowercase as we observed in our logs.

Thanks for the deep analysis! We have a different ticket open for using geventhttpclient’s UserAgent #2681

Do you think that would that solve this issue?

I think the problem is with FastHttpSession relying on LocustUserAgent's and thus geventhttpclient UserAgent's internal implementation, namely _make_request. Though as I said above, I need to investigate this further, as _make_request did not behave as I expected. This could still be my mistake in the script, even though the commit I posted above seemed to fix the problem.

On a side note, it looks like LocustUserAgent overrides geventhttpclient UserAgent's _urlopen, even though the implementation seems identical.

@cyberw
Copy link
Collaborator

cyberw commented Oct 10, 2024

Yea the implementation in Locust was copied over to geventhttpclient (because it could be useful to others as well, not just Locust :)

Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Issue had no activity. Might still be worth fixing, but dont expect someone else to fix it label Dec 10, 2024
Copy link

This issue was closed because it has been stalled for 10 days with no activity. This does not necessarily mean that the issue is bad, but it most likely means that nobody is willing to take the time to fix it. If you have found Locust useful, then consider contributing a fix yourself!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug stale Issue had no activity. Might still be worth fixing, but dont expect someone else to fix it
Projects
None yet
Development

No branches or pull requests

2 participants