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

Subsequent removal and addition of a cookie may leave the cookie removed #2084

Closed
roganov opened this issue Jul 12, 2017 · 4 comments
Closed
Labels
Milestone

Comments

@roganov
Copy link
Contributor

roganov commented Jul 12, 2017

Long story short

I encountered a flaky test failure when testing login-logout-login and discovered that a login right after a logout may not work, in other words, the authentication cookie may not be set.

This boiled down to the following error in CookieJar: for event loops with poor time resolution (in my case uvloop), subsequent removal and addition of a cookie may not add the cookie.

Here is a test case:

import time
from http.cookies import SimpleCookie

from aiohttp.cookiejar import CookieJar
from uvloop import new_event_loop


def test_uvloop_cookies():
    loop = new_event_loop()

    for _ in range(1000):
        jar = CookieJar(unsafe=True, loop=loop)

        # Remove`test` cookie.
        jar.update_cookies(SimpleCookie('test=""; expires=Thu, 01 Jan 1970 00:00:00 GMT; Max-Age=0; Path=/'))
        # Set `test` cookie to `bar`.
        jar.update_cookies(SimpleCookie('test="bar"'))

        # Assert that there a cookie.
        assert list(jar)
        time.sleep(0.1)
        assert list(jar)

Although it took me quite a bit of time to discover what's going on, it seems there is an extremely simple fix: replace strict comparison (<) with non-strict one (<=) here.
I'll provide a PR soon if maintainers agree.

Expected behaviour

Test passes

Actual behaviour

Test fails

Steps to reproduce

Run the test

Your environment

uvloop=0.8.0, aiohttp=2.1.0

@asvetlov
Copy link
Member

Would you make a PR (unittest required)?

@roganov
Copy link
Contributor Author

roganov commented Jul 13, 2017

Yes, I'll submit by tomorrow hopefully.

roganov added a commit to roganov/aiohttp that referenced this issue Jul 13, 2017
roganov added a commit to roganov/aiohttp that referenced this issue Jul 13, 2017
roganov added a commit to roganov/aiohttp that referenced this issue Jul 13, 2017
roganov added a commit to roganov/aiohttp that referenced this issue Jul 13, 2017
roganov added a commit to roganov/aiohttp that referenced this issue Jul 13, 2017
@asvetlov asvetlov added this to the 2.3.0 milestone Jul 13, 2017
roganov added a commit to roganov/aiohttp that referenced this issue Jul 14, 2017
roganov added a commit to roganov/aiohttp that referenced this issue Jul 14, 2017
@asvetlov
Copy link
Member

Fixed by #2088

@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.

@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.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants