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

Catch concurrent.futures.CancelledError in websocket code. #12150

Merged
merged 2 commits into from
Feb 7, 2018

Conversation

pelson
Copy link
Contributor

@pelson pelson commented Feb 3, 2018

Description:

It isn't clear why a concurrent.futures.CancelledError is appearing instead of an asyncio.CancelledError, but #9546 shows that it is (or at least, has done when the connection timed out).

Related issue (if applicable): fixes #9546.

Checklist:

  • The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@pvizeli
Copy link
Member

pvizeli commented Feb 3, 2018

@balloob maybe we use future.CancelledError so that is clear to other one?

@pvizeli pvizeli requested a review from balloob February 6, 2018 11:27
@balloob
Copy link
Member

balloob commented Feb 6, 2018

I agree with Pascal. Please do from concurrent import futures and please add a comment with a link to the issue for future reference.

I wonder if we should check this upstream to see if it has either been addressed already or is a known issue?

@balloob balloob merged commit 5ba02c5 into home-assistant:dev Feb 7, 2018
@balloob
Copy link
Member

balloob commented Feb 7, 2018

Awesome 👍

@pelson pelson deleted the fix_9546 branch February 7, 2018 09:24
@balloob balloob mentioned this pull request Feb 9, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aiohttp errors
4 participants