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

Fix aiohttp upgrade #951

Merged
merged 2 commits into from
Oct 15, 2019
Merged

Conversation

cognifloyd
Copy link
Contributor

Fix test_run_with_aiohttp_not_installed with aiohttp 3.5.2+.

The error was prevented by requiring aiohttp<3.5.2 in #844.
Somewhere between 3.5.1 and 3.5.2, this aiohttp reimport started succeeding.
I used pycharm's debugger to determine that it is this import that was successfully re-importing aiohttp.
https://github.com/zalando/connexion/blob/ff3adb393c67dea51a735ef3211794084db3623e/connexion/cli.py#L151-L155

It's not clear which change caused the issue, but I would guess one of these:

Changes proposed in this pull request:

Somewhere between 3.5.1 and 3.5.2, the aiohttp reimport started
succeeding (in connexion.connexion.cli.run()). It's not clear which
change caused the issue, but it's probably one of:

- aio-libs/aiohttp#3469 (Remove wildcard imports)
- aio-libs/aiohttp#3464 (Don't suppress gunicorn cleanup errors)
- aio-libs/aiohttp#3471 (Refactor workers)
- aio-libs/aiohttp#3500 (Ignore done tasks)

In any case, setting sys.modules['aiohttp'] = None should prevent
reimporting it. See: https://stackoverflow.com/a/1350574

I successfully tested locally on py37 with aiohttp 3.5.1 and 3.5.2.
@cognifloyd
Copy link
Contributor Author

@dtkav wrote #844 and @hjacobs approved it. What do you think of the fix here so that we revert #844?

@kornicameister
Copy link
Contributor

Yeah, they some things happening regarding imports and so on.

@hjacobs
Copy link
Contributor

hjacobs commented Oct 15, 2019

👍

@hjacobs hjacobs merged commit bd2552c into spec-first:master Oct 15, 2019
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.

3 participants