Raise ConnectionResetError 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

• I think the code is well written
• 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]

Merging this PR will not alter performance

✅ 59 untouched benchmarks

---

_{Comparing agners:use-runtime-exception-if-transport-is-none (e826df7) with master (46e4cad)}

[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): home-assistant/supervisor#6241, specifically home-assistant/supervisor#6241 (comment).

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.

[agners]

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.

I've added such a test, along with using an asyncio.Event to strictly enforce the required interleaving.

[Dreamsorcerer]

I've added such a test, along with using an asyncio.Event to strictly enforce the required interleaving.

Perfect, thanks for that. I'll have a little play with it later and see if we can get rid of the None checks.

[Dreamsorcerer]

I had a little go in #12244. It's pretty awkward to remove the None entirely, as you'd be left with an AttributeError until connection_made() is called to set the transport initially. I think we'll just leave it as is for now.

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.10%. Comparing base (46e4cad) to head (e826df7).
⚠️ Report is 3 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
aiohttp/web_fileresponse.py	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11761      +/-   ##
==========================================
- Coverage   99.10%   99.10%   -0.01%     
==========================================
  Files         130      130              
  Lines       45402    45425      +23     
  Branches     2399     2401       +2     
==========================================
+ Hits        44997    45019      +22     
  Misses        273      273              
- Partials      132      133       +1     

Flag	Coverage Δ
CI-GHA	98.96% <92.30%> (-0.01%)	⬇️
OS-Linux	98.70% <92.30%> (-0.01%)	⬇️
OS-Windows	96.97% <92.30%> (-0.01%)	⬇️
OS-macOS	97.86% <92.30%> (-0.01%)	⬇️
Py-3.10.11	97.41% <92.30%> (-0.01%)	⬇️
Py-3.10.19	97.89% <92.30%> (-0.01%)	⬇️
Py-3.10.20	?
Py-3.11.14	97.42% <92.30%> (-0.54%)	⬇️
Py-3.11.15	97.95% <92.30%> (+0.53%)	⬆️
Py-3.11.9	97.62% <92.30%> (-0.01%)	⬇️
Py-3.12.10	97.70% <92.30%> (-0.01%)	⬇️
Py-3.12.12	97.51% <92.30%> (-0.67%)	⬇️
Py-3.12.13	98.04% <92.30%> (-0.15%)	⬇️
Py-3.13.12	98.42% <92.30%> (-0.01%)	⬇️
Py-3.14.3	98.48% <92.30%> (-0.01%)	⬇️
Py-3.14.3t	97.49% <92.30%> (+<0.01%)	⬆️
Py-pypy3.11.13-7.3.20	97.52% <92.30%> (-0.01%)	⬇️
VM-macos	97.86% <92.30%> (-0.01%)	⬇️
VM-ubuntu	98.70% <92.30%> (-0.01%)	⬇️
VM-windows	96.97% <92.30%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Dreamsorcerer]

Is there a version of the test that allows us to cover this line?