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

'None' in HTTP headers #2183

Closed
nesdis opened this issue Aug 9, 2017 · 14 comments
Closed

'None' in HTTP headers #2183

nesdis opened this issue Aug 9, 2017 · 14 comments
Assignees
Milestone

Comments

@nesdis
Copy link

nesdis commented Aug 9, 2017

Long story short

Sometimes in file cookies.py the rawdata is of type None and above exception is hit.

def load(self, rawdata):
    """Load cookies from a string (presumably HTTP_COOKIE) or
    from a dictionary.  Loading cookies from a dictionary 'd'
    is equivalent to calling:
        map(Cookie.__setitem__, d.keys(), d.values())
    """
    if isinstance(rawdata, str):
        self.__parse_string(rawdata)
    else:
        # self.update() wouldn't call our custom __setitem__
        for key, value in rawdata.items():
            self[key] = value
    return

Expected behaviour

Why is raw_data given to load of type None?

This is called in function start of C:\Python36\Lib\site-packages\aiohttp\client_reqrep.py

for hdr in self.headers.getall(hdrs.SET_COOKIE, ()):
        try:
            self.cookies.load(hdr)
        except CookieError as exc:
            client_logger.warning(
                'Can not load response cookies: %s', exc)
    return self

Actual behaviour

Looks like self.headers.getall(hdrs.SET_COOKIE, ()) is returning hdr of type None, This looks buggy. It should return actual header or raise stopiteration. At times it returns None.

Steps to reproduce

I have not been able to reproduce this issue. Unfortunately I lost the stack trace console output too. But have debugged till the above point.

Your environment

@asvetlov
Copy link
Member

asvetlov commented Aug 9, 2017

No idea how did you get it.
Looks like some code explicitly pushed None into headers but aiohttp doesn't do it.

Reproducible example would help very much.

@asvetlov
Copy link
Member

asvetlov commented Sep 1, 2017

Multidict doesn't return None if key doesn't exist but some code might push None explicitly.
I pretty sure aiohttp doesn't to it itself.

Closing the issue.
Feel free to open new one if you'll get more info.

@asvetlov asvetlov closed this as completed Sep 1, 2017
@asvetlov asvetlov added the invalid This doesn't seem right label Sep 1, 2017
@gilbsgilbs
Copy link
Contributor

@asvetlov I'm also affected by this issue, and I'm 100% sure I don't explicitly alter response headers in my code (I don't think I even could because this happens in the post function directly). I'm unable to reproduce locally, but it did happen in production a couple of times (and only one of the remote hosts have produced the issue).

Traceback (most recent call last):
  File ………
    timeout=timeout,
  File "python3.5/site-packages/aiohttp/client.py", line 636, in __aenter__
    self._resp = yield from self._coro
  File "python3.5/site-packages/aiohttp/client.py", line 240, in _request
    yield from resp.start(conn, read_until_eof)
  File "python3.5/site-packages/aiohttp/client_reqrep.py", line 585, in start
    self.cookies.load(hdr)
  File "python3.5/http/cookies.py", line 559, in load
    for key, value in rawdata.items():
AttributeError: 'NoneType' object has no attribute 'items'

Using aiohttp 2.1.0 and python 3.5. The remote server seems to include some headers in its response, including three Set-Cookie. Unfortunately, I can't get the headers value.

Since I got the exception in Sentry, I can provide some more information.

http/cookies.py in load at line 559

rawdata None
self {'SomeString-md5hash': {'comment': '', 'domain': 'www.example.co', 'expires': 'Sun, 24-Sep-2017 13:59:17 GMT', 'httponly': True, 'max-age': '', 'path': '/', 'secure': '', 'version': ''}}

aiohttp/client_reqrep.py in start at line 585

connection Connection<('www.example.co', 80, False)>
hdr None
payload <FlowControlStreamReader 30 bytes eof>
read_until_eof True

aiohttp/client.py in _request at line 240

allow_redirects False
auth None
chunked None
compress None
conn Connection<('www.example.co'', 80, False)>
cookies {'PHPSESSID': {'comment': '', 'domain': '', 'expires': '', 'httponly': '', 'max-age': '', 'path': '', 'secure': '', 'version': ''}, 'SomeString-md5hash': {'comment': '', 'domain': '', 'expires': '', 'httponly': '', 'max-age': '', 'path': '', 'secure': '', 'version': ''}, 'SomeString-anothermd5hash': {'comment': '', 'domain': '', 'expires': '', 'httponly': '', 'max-age': '', 'path': '', 'secure': '', 'version': ''}}
data [censored]
encoding None
expect100 False
handle None
headers <CIMultiDict('User-Agent': 'MyUser-Agent', 'Content-Type': 'application/json')>
history []
json None
max_redirects 10
method 'POST'
params None
proxy None
proxy_auth None
read_until_eof True
redirects 0
req <aiohttp.client_reqrep.ClientRequest object at 0x7fbb1d7a57b8>
self <aiohttp.client.ClientSession object at 0x7fbb2072c208>
skip_auto_headers None
skip_headers []
timeout 300
timer <aiohttp.helpers.TimerContext object at 0x7fbb1d7a56d8>
tm <aiohttp.helpers.TimeoutHandle object at 0x7fbb1f037198>
url URL('http://www.example.co/some/path')
version [1, 1]

Do not hesitate if you need further information. Hope this helps. Thanks.

@asvetlov asvetlov reopened this Sep 11, 2017
@asvetlov
Copy link
Member

I'm able to reproduce the issue. Fetch from https://reload.dk fails: it splits a header into two, one of them has None as a value.
Pure Python parser works ok but C accelerator fails.
Working on making reliable test.

@asvetlov asvetlov self-assigned this Sep 20, 2017
@asvetlov asvetlov added bug and removed invalid This doesn't seem right labels Sep 20, 2017
@fafhrd91
Copy link
Member

There are some new commits for http-parse, we should try latest c code

@asvetlov
Copy link
Member

It's not release yet.
At first I'd like to check conformance to the following table:

(on_header_field and on_header_value shortened to on_h_*)
 ------------------------ ------------ --------------------------------------------
| State (prev. callback) | Callback   | Description/action                         |
 ------------------------ ------------ --------------------------------------------
| nothing (first call)   | on_h_field | Allocate new buffer and copy callback data |
|                        |            | into it                                    |
 ------------------------ ------------ --------------------------------------------
| value                  | on_h_field | New header started.                        |
|                        |            | Copy current name,value buffers to headers |
|                        |            | list and allocate new buffer for new name  |
 ------------------------ ------------ --------------------------------------------
| field                  | on_h_field | Previous name continues. Reallocate name   |
|                        |            | buffer and append callback data to it      |
 ------------------------ ------------ --------------------------------------------
| field                  | on_h_value | Value for current header started. Allocate |
|                        |            | new buffer and copy callback data to it    |
 ------------------------ ------------ --------------------------------------------
| value                  | on_h_value | Value continues. Reallocate value buffer   |
|                        |            | and append callback data to it             |
 ------------------------ ------------ --------------------------------------------

Maybe you've missed correct processing of Previous name continues state?

@fafhrd91
Copy link
Member

interesting. yes, multiple field callbacks are not supported
https://github.com/aio-libs/aiohttp/blob/master/aiohttp/_http_parser.pyx#L139

@asvetlov
Copy link
Member

@fafhrd91 have you a time for working on PR or should I take the duty?

@fafhrd91
Copy link
Member

I'll fix it.

@fafhrd91 fafhrd91 assigned fafhrd91 and unassigned asvetlov Sep 20, 2017
@fafhrd91 fafhrd91 added this to the 2.3 milestone Sep 20, 2017
@asvetlov
Copy link
Member

Cool! Thanks!

@fafhrd91
Copy link
Member

@nesdis @gilbsgilbs could you confirm if problem fixed in master?

@gilbsgilbs
Copy link
Contributor

Thank you very much @fafhrd91 and @asvetlov for looking into this.

Since I was not able to reproduce the issue locally, I cannot guarantee it fixes my use-case. Anyways, as soon as this is released, I'll deploy to production and after a while get some confidence on whether this is fixed or not. The issue reproduces a few times per week.

@asvetlov
Copy link
Member

Let's assume the problem is fixed by #2281
Feel free to create a new issue if needed.

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants