Raise RuntimeError if transport is None

[agners]

What do these changes do?

When a client disconnects immediately after connecting, the transport might become None. This should not lead to an assertion, since this can happen in real-world scenarios. Use a RuntimeException instead.

Are there changes in behavior for the user?

A WebSocket connection disconnecting early will no longer lead to a assertion but a RuntimeException instead.

Is it a substantial burden for the maintainers to support this?

Related issue number

Checklist

• [x] I think the code is well written
• [x] Unit tests for the changes exist
• [ ] Documentation reflects the changes
• [ ] If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• [ ] Add a new news fragment into the CHANGES/ folder

  • name it <issue_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)

  • if you don't have an issue number, change it to the pull request number after creating the PR

    • .bugfix: A bug fix for something the maintainers deemed an improper undesired behavior that got corrected to match pre-agreed expectations.
    • .feature: A new behavior, public APIs. That sort of stuff.
    • .deprecation: A declaration of future API removals and breaking changes in behavior.
    • .breaking: When something public is removed in a breaking way. Could be deprecated in an earlier release.
    • .doc: Notable updates to the documentation structure or build process.
    • .packaging: Notes for downstreams about unobvious side effects and tooling. Changes in the test invocation considerations and runtime assumptions.
    • .contrib: Stuff that affects the contributor experience. e.g. Running tests, building the docs, setting up the development environment.
    • .misc: Changes that are hard to assign to any of the above categories.

  • Make sure to use full sentences with correct case and punctuation, for example:

    Fixed issue with non-ascii contents in doctest text files
    -- by :user:`contributor-gh-handle`.
    

    Use the past tense or the present tense a non-imperative mood, referring to what's changed compared to the last released version of this project.

[codspeed-hq[bot]]

CodSpeed Performance Report

Merging #11761 will not alter performance

_{Comparing agners:use-runtime-exception-if-transport-is-none (9f20389) with master (056d929)}

Summary

✅ 59 untouched

[Dreamsorcerer]

The assertion is because the code expects it to never be None in this scenario, so I'm not sure this is the correct fix. I'll use your test and try to figure out if there's a better solution later.

[Dreamsorcerer]

I'll use your test and try to figure out if there's a better solution later.

Wait, sorry, your test literally just assigns the transport to None. We need the actual set of steps that causes this to happen in production.

[Dreamsorcerer]

Running through the code, I think the steps that may reproduce this would look like:

• Client creates a connection.
• Handler pauses (asyncio.sleep() for a test)
• Client disconnects
• Handler calls ws.prepare().

I think the WebSocketWriter that is returned from .prepare() is responsible for raising exceptions related to the client disconnecting. Therefore, my first thought is that we should probably figure out a fix that ensures the (potentially closed) transport is always available inside .prepare().

[Dreamsorcerer]

I think setting the transport to None is something that has been copied from asyncio, but I'm not clear if there's a real reason for that. Maybe we can just avoid setting to None, which would greatly simplify our type checking. @bdraco Any thoguhts?

[agners]

Wait, sorry, your test literally just assigns the transport to None. We need the actual set of steps that causes this to happen in production.

Yeah sorry, I realize this test is not ideal, but triggering a race condition is also not really a good idea for a pytest.

Running through the code, I think the steps that may reproduce this would look like:

Right, I think that is pretty much the sequence we see in production. I have a reproducer which allows to trigger the bug in our use case (Home Assistant Supervisor): https://github.com/home-assistant/supervisor/pull/6241, specifically https://github.com/home-assistant/supervisor/pull/6241#issuecomment-3381392252.

I think setting the transport to None is something that has been copied from asyncio, but I'm not clear if there's a real reason for that. Maybe we can just avoid setting to None, which would greatly simplify our type checking. @bdraco Any thoguhts?

Hm, I guess that would make the transport to raise a closed connection exception or similar, this sounds like a good approach to me.

[Dreamsorcerer]

triggering a race condition is also not really a good idea for a pytest

Following the steps I outlined, we should be able to reproduce it with minimal timing concerns. We have many similar tests that reproduce other race conditions.